dwsmith1983 commented on PR #5365: URL: https://github.com/apache/datafusion-comet/pull/5365#issuecomment-5521710321
Thanks for the breadth here. Two of these were defects worth fixing in the PR, done and pushed in 48e5d2902; the rest are filed as tracking issues per your suggestion. Point by point: **Calendar handling: fixed in-PR.** Your repro was real and it led somewhere interesting: the regular native scan has no rebase handling at all today (that is documented as a known limitation and tracked in #5010, ParquetRebaseDatetimeSuite is disabled for Comet), so there was nothing to inherit. The Delta arm now resolves calendar policy per file on the native side: the footer's writer metadata (spark version, legacyDateTime, legacyINT96, timeZone) survives into the Arrow schema the expression adapter sees per file, and it resolves date and timestamp policies the way DataSourceUtils.getRebaseSpec does. Dates rebase exactly (Spark's julianGregDiffs table ported verbatim, with calendar arithmetic before its range). Timestamps rebase exactly for UTC writer zones. Anything it cannot rebase faithfully (non UTC writer zone, missing metadata, contradictory flags) refuses ancient values with an error naming the column instead of returning shifted data; modern values always pass. Your exa ct 1500-01-01 and ancient INT96 cases are regression tests now, RED before the fix with your 1500-01-10 shift reproduced, plus mixed writer metadata and a temporal predicate case. The mechanism is gated to the Delta arm so the regular scan's documented behavior is unchanged, pinned by a control test. **Assumed-role session policies: fixed in-PR.** fs.s3a.assumed.role.policy now declines admission, global and bucket scoped, resolved exactly like its consumer (AssumedRoleCredentialProvider reads it with getTrimmed on the propagated conf and sends it in the AssumeRole request, verified against 3.3.4 and 3.4.1 bytecode; native never forwards it). The reason text names the key without echoing the policy document. Unset stays admitted, pinned. **Tracking issues filed** for the rest: DV file splitting #5655, compressed DV decoding #5656 (a candidate patch preserving sub bitmap containers already exists from a community contributor and can be adapted once this lands), overlapping bitmap and footer reads #5657, shared cloud compatibility helper #5658, credential scoping #5659, v2 checkpoint depth #5660, capability table #5661. **Benchmark on this head** (48e5d2902 exactly, no extra commits; Apple M5, JDK 17, spark-3.5 profile, local FS, 120M rows, 6 files of ~490MB, zstd, medians of 5 warm runs per fresh session; results bit identical across all modes and verified against closed form expectations): | pattern | deleted | stock | comet fallback | native | |---|---|---|---|---| | none | 0 | 5.31s | 5.25s | 1.13s | | sparse (0.1% scattered) | 120K | 7.76s | 5.35s | 1.52s | | contiguous (20%) | 24M | 5.76s | 2.77s | 1.22s | | alternating (50%) | 60M | 4.76s | 2.53s | 2.51s | Native output_rows confirms DV application inside the scan (119,880,000 on sparse). Preparation cost measured on the real exported DVs through the PR's own decode and access plan code: decode is negligible everywhere (CRC plus roaring, up to 0.36ms per file on the 2.5MB alternating DVs); the cost center is selector expansion for alternating deletes, 61 to 93ms and ~400MB true peak per file, about 0.5s CPU across the table. Contiguous deletes cost near zero allocation since fully deleted row groups become skips. Two operational notes the alternating case surfaced. First, the default dv.maxDeletedRowsPerFile cap (1M) declines contiguous and alternating up front and falls back cleanly; the numbers show that guard picks the better path for alternating (native only ties fallback there while needing a ~1.6GB reservation per file). Second, raising the cap without sizing the off heap pool fails tasks hard at the reservation, which is the documented reserve before build behavior doing its job; 18g admits it. The reservation is about 4x the measured true peak, deliberately pessimistic. Suite batteries at this head pass 227/227 on both spark-3.5 and spark-4.0, MinIO suite live. The remaining gap from your list is live object store validation beyond MinIO, which I do not have infrastructure for here. -- 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]
