andygrove commented on PR #5407: URL: https://github.com/apache/datafusion-comet/pull/5407#issuecomment-5441852379
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. > Scope note: this is a 4,600-line change and I have reviewed it structurally rather than line by line. @sunchao has already done several detailed passes, so I have tried to stick to things a structural read surfaces. The description is genuinely excellent. Every claim about Spark's and Arrow-rs's behavior is anchored to a specific file and line range at a pinned version, and the reasoning for why the physical Struct shape alone is insufficient (the logical Field marker, the `[value, metadata]` ordinal contract, `ColumnVector.getVariant` reading children positionally) is laid out clearly enough that a reviewer can check it. The follow-up issues #5474, #5477, and #5495 for the compatibility rewrites you are deliberately carrying is the right way to keep temporary code from becoming permanent. Four things. **The FFI and Arrow-bridge changes are not Variant-specific** Switching the FFI exporter from `TryFrom<&DataType>` to `TryFrom<&Field>` so name, nullability, and metadata survive, and teaching `Utils.fromArrowField` that not every Arrow Struct is a Spark `StructType`, are changes to shared boundaries that every Comet type crosses. What is the blast radius? Specifically: does exporting the top-level name and metadata change anything for types that previously exported bare, for example a struct column whose Arrow field carries metadata Comet did not previously ship across? And does the JVM side now key off metadata in a way that could misclassify a user struct? #5292 is making an analogous change for calendar intervals with a tagged struct plus a metadata marker, and the two are going to interact in `Utils.scala`. Worth coordinating. **Per-scan Arrow-schema policy** Replacing the embedded `ARROW:schema` footer hint with a physical-schema-derived one, and narrowing Decimal256 to Decimal128 at precision at most 38, are described as scoped to "an unencrypted projected Variant". That scoping is the load-bearing part. Is it enforced structurally, or is it a condition that a future refactor could widen by accident? If a non-Variant scan ever took this path, replacing the writer's schema hint would change how ordinary columns are read. #5477 tracks reassessing this, which is good, but a comment at the decision point saying exactly which condition gates it would help. **Test-to-code ratio and where the risk sits** 1,419 lines of `variant.rs` with 1,226 lines of tests plus 549 lines in `ParquetReadSuite` is a healthy ratio. The cases I would most want covered, and would like pointed at in the description: a shredded Variant where `typed_value` is present and `value` is absent, the reverse, both present (the reconstruction precedence), a Variant column that is entirely null, and a Variant nested inside a struct or array that should *not* be projected by this phase and must fall back cleanly rather than half-work. **The UTF-16 rewrite** #5474 tracks removing the Spark UTF-16 ordering compatibility rewrite once every supported Spark profile has the upstream UTF-8 fix. While it is in place, is it applied only to Variant values or to strings generally? If the latter, that is another shared-boundary change worth calling out separately, since it would affect ordering semantics beyond this feature. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
