dwsmith1983 commented on PR #5365: URL: https://github.com/apache/datafusion-comet/pull/5365#issuecomment-5522992607
Thanks for the depth, and for the independent verification of the DV decode path. Timing note first: your review and my last push crossed. 053ec14b4 (pushed about an hour after your review) plumbs the effective session read modes into the scan, so the resolve_spec comment you quoted is gone: metadata-free files now follow the resolved datetimeRebaseModeInRead and int96RebaseModeInRead exactly as ParquetFileFormat resolves them (CORRECTED reads verbatim, LEGACY rebases, EXCEPTION refuses ancient), and the nested branch now checks the applicable leaf policy, so STRUCT with a DATE leaf under corrected dates plus legacy INT96 stays native. Both of your reproductions are regression tests on raw parquet-mr files with no Spark metadata. On the sharper point that refusals bypass the decline mechanism: after the modes fix the residual execution-time refusal class is much narrower, but it is not empty and I want to be precise about what remains. It fires only when ancient values actually appear in a file whose policy resolved LEGACY with a non-UTC or unrecorded writer zone, or with contradictory legacy flags. Under EXCEPTION mode an execution-time failure matches Spark's own behavior for those values, so the true fallback-bypass is the LEGACY non-UTC sliver, where Spark succeeds using zone tables native does not have. Declining at planning time requires knowing the file's writer zone, which the JVM side cannot see without reading footers; the conservative alternative is declining every scan whenever an effective mode is LEGACY, which over-declines tables that are entirely modern or UTC-written and would give up the native path for data that reads correctly. That is a genuine tradeoff rather than an oversight, and I am happy to implement the planning-time decline for LEGACY modes if you and @sunchao prefer safety-of-mechanism over coverage here; it is a small change to the existing gate. Core-path gaps: agreed on both, and they deserve to exist independently of this PR. The shared comparator extraction was already filed as #5658; I have now filed #5662 for the concrete correctness gap you named (the regular native scan forwarding S3 options with no divergence check). The calendar gap for the plain path is #5010, and the rebase module here was built so it can be lifted wholesale when core wants it; the delta-arm flag is the isolation seam, not a design commitment. On delta in default cargo features: the motivation is exactly what you inferred, letting the contrib jar work against stock Comet binaries so opting in is a Spark-side decision rather than a custom native build. I agree the philosophy conflict with the adjacent contrib-delta comment and #5411 should be settled explicitly rather than by default; I am fine either way and will move it out of default if that is the call. Same for sequencing against #4366, which is yours and sunchao's decision to make; happy to record whatever is agreed in the Cargo.toml comment and the PR description. -- 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]
