sunchao commented on PR #5365: URL: https://github.com/apache/datafusion-comet/pull/5365#issuecomment-5520366084
Follow-up review of `512c9521`. The revised scope is easier to review. Reusing the shared Unicode implementation and separating the independent core fixes leaves a clearer Delta integration. The following items cover correctness, performance, design, and validation. They can be developed as separate changes, with tracking issues linked for follow-up work. ### Calendar handling Could the reader resolve calendar policy from each file’s writer metadata and apply rebasing or conservative fallback? Fresh end-to-end tests against this head reproduced a LEGACY-written DATE `1500-01-01` reading as `1500-01-10` through native Delta. An ancient INT96 timestamp shifted by the same nine days. Modern INT96 and corrected-calendar controls passed, with native execution asserted. The existing INT96 test uses modern timestamps and therefore does not exercise rebasing. Useful regressions would cover ancient values, mixed writer metadata, and temporal predicates. Setting the read mode to `CORRECTED` alone is insufficient because Spark’s file metadata can override that setting. ### Assumed-role session policies The provider compatibility check could cover configured session restrictions in addition to provider class names. Fresh component tests show that global and bucket-specific `fs.s3a.assumed.role.policy` settings pass admission. Hadoop 3.4.1 includes this policy in its STS request, while the native provider does not forward it. Switching to native therefore drops the configured session restriction. Declining these configurations until native support is available would preserve the contract. Global, bucket-specific, and unset-policy controls would pin the behavior. This finding was verified through current-source admission tests and the actual Hadoop implementation, without a live STS request. ### DV file splitting and split-aware preparation A separate improvement could enable splitting large DV-backed files to increase scan parallelism. The current admitted DV path carries Spark’s row-index field, which causes the shared planner to disable file splitting. Consequently, repeated whole-file preparation across ordinary DV splits is not an established problem in the current execution path. If native DV splitting is enabled, access-plan construction should prepare only owned row groups while preserving file-global row positions, whole-file corruption validation, and memory accounting. Dictionary-page ownership, half-open boundaries, and interaction with pruning deserve explicit regressions. Descriptor reuse can then be evaluated where multiple splits actually share a DV. ### Compressed deletion-vector decoding The native-format decoder currently enumerates each deleted position and reinserts it into a treemap. Preserving compressed bitmap containers during conversion could avoid that additional pass. Evaluate this with sparse, contiguous, and alternating deletions, measuring decoding time and allocations. Portable-format decoding already uses direct deserialization. This optimization would not eliminate the later iteration needed to construct partial-row-group selections. ### Overlapping bitmap and footer reads For on-disk DVs, bitmap and Parquet footer reads are independent, but preparation currently waits for bitmap fetching, decoding, and validation before starting the footer read. Overlapping these operations could reduce remote-storage preparation latency. Retain bounded concurrency across files and preserve cancellation, error handling, and memory reservations. Measure peak memory alongside latency because overlapping operations can increase concurrent resource use. Inline DVs have no bitmap network read to overlap. ### Shared cloud compatibility checks Cloud compatibility policy could live in a focused helper beside the shared object-store configuration code. Delta admission would then concentrate on protocol features, schemas, and Delta metadata. Both native Parquet and native Delta use the same underlying client, so shared checks would help compatibility fixes apply consistently. Preserve Hadoop’s distinct rules for plain options, password-backed values, variable substitution, and bucket overrides. This should simplify existing decisions without introducing another provider abstraction. ### Credential lifecycle and scope Could credential extraction happen during execution-payload construction, after final file selection, and include only the required data-file and DV authorities? The current planning payload retains options selected by provider-wide prefixes. Deferring and scoping extraction would reduce credential retention and unnecessary payload content. Tests could verify that planning payloads contain no credentials, execution payloads contain only the selected scopes, and credentials required by external DV locations remain available. No credential leak or performance improvement was demonstrated by this review. ### v2-checkpoint regression coverage The v2-checkpoint test could verify actual checkpoint artifacts, include a sidecar-backed case, invalidate the Delta log cache, and reopen the snapshot before comparing Spark and native results. The current fixture performs several commits and then reads immediately. It does not establish that the read reconstructs the snapshot from the checkpoint rather than using cached state. A post-checkpoint append and time-travel read would further exercise the boundary between Delta’s snapshot reconstruction and native table-data scanning. ### Supported-feature documentation A capability table would make the experimental reader’s scope easier to understand. It should describe native support and fallback boundaries for ID mapping, mapped nested schemas, CDF, and generated row metadata. Clarify that constant file metadata and consumed row indexes have different support rules. Link follow-up issues for additional coverage so users can distinguish deliberate fallback from planned extensions. ### Benchmarks and validation A focused benchmark should cover large files and sparse, contiguous, and alternating deletion patterns. Report preparation time, decoding allocations, reserved and peak memory, total scan time, output equality, and confirmed native execution. Split-file and descriptor-reuse benchmarks should accompany any change enabling native DV splitting. The previously published benchmark used another head plus optimization commits, so it does not establish this revision’s performance. Fresh validation on Spark 4.0.4 / Delta 4.0.1 passed 216 existing Delta tests. Three MinIO tests were canceled because Docker was unavailable. Additional checks passed 386 DV component scans and 30 planning probes. The separate historical-calendar regressions reproduced the mismatches described above. Results for the remaining supported Spark profiles and live object-store tests would complete the validation picture. GitHub reported no CI checks attached to this head at review time. -- 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]
