[GLUTEN-12538][VL] Unblock TIMESTAMP_NTZ min/max in Delta statistics - #12967
felipepessoto wants to merge 10 commits into
Conversation
|
Run Gluten Clickhouse CI on x86 |
|
🔄 Delta Spark UT started by @felipepessoto (~2.5 h). View run |
|
Run Gluten Clickhouse CI on x86 |
2 similar comments
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🟡 Changes recommended
A newly added Velox round-trip test uses millisecond-precision timestamps despite declaring microsecond precision, weakening coverage for the intended NTZ precision behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Enables native handling of TIMESTAMP_NTZ (Spark) / TIMESTAMP_UTC (Velox) in aggregation and related Delta statistics collection paths for the Velox backend, reducing Spark fallbacks and unblocking Delta stats plans previously rejected due to NTZ types.
Changes:
- Extend the
TimestampNTZfallback validator to allow aggregates, shuffles, and direct NTZ projections needed by Delta stats plans. - Accept
TimestampNTZTypein aggregation buffer type checks and preserve a distinct native signature token (tsntz) plus SubstraitPrecisionTimestampencoding. - Add native regression coverage (Spark UT + Velox C++ tests) and remove now-fixed Delta data-skipping cases from the known-failure baseline.
File summaries
| File | Description |
|---|---|
| gluten-ut/spark41/src/test/scala/org/apache/spark/sql/GlutenTimestampNtzAggregateSuite.scala | Adds Spark 4.1 regression coverage for NTZ min/max aggregation and a projection fallback case. |
| gluten-ut/spark41/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala | Enables the new Spark 4.1 UT suite in Velox test settings. |
| gluten-substrait/src/main/scala/org/apache/gluten/extension/columnar/validator/Validators.scala | Broadens NTZ fallback validation to permit additional plan nodes/expressions used by native stats aggregation. |
| gluten-substrait/src/main/scala/org/apache/gluten/expression/ConverterUtils.scala | Maps Spark TimestampNTZType to Substrait timestamp NTZ type node and uses tsntz in signature naming. |
| gluten-substrait/src/main/scala/org/apache/gluten/execution/HashAggregateExecBaseTransformer.scala | Allows TimestampNTZType in supported aggregation buffer type checks. |
| cpp/velox/tests/VeloxToSubstraitTypeTest.cc | Adds a type-conversion test for TIMESTAMP_UTC -> Substrait PrecisionTimestamp. |
| cpp/velox/tests/VeloxSubstraitSignatureTest.cc | Adds tsntz signature coverage for TIMESTAMP_UTC mapping in both directions. |
| cpp/velox/tests/VeloxSubstraitRoundTripTest.cc | Adds a min/max aggregation round-trip test over TIMESTAMP_UTC. |
| cpp/velox/substrait/VeloxToSubstraitType.cc | Encodes TIMESTAMP_UTC as Substrait precision_timestamp(6). |
| cpp/velox/substrait/VeloxSubstraitSignature.cc | Maps TIMESTAMP_UTC to/from the tsntz signature token. |
| backends-velox/src-delta40/test/scala/org/apache/spark/sql/delta/GlutenDeltaStatsSuite.scala | Adds Delta 4.0 stats regression coverage including nested NTZ columns. |
| backends-velox/src-delta33/test/scala/org/apache/spark/sql/delta/GlutenDeltaStatsSuite.scala | Adds Delta 3.3 stats regression coverage including nested NTZ columns. |
| .github/workflows/util/delta-spark-ut/known-failures.txt | Removes Delta data-skipping known failures resolved by native NTZ stats aggregation support. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@Mariamalmesfer @rui-mo I think you two have worked on other timestamp_ntz PRs. Could you take a look, please? |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
gluten-substrait/src/main/scala/org/apache/gluten/extension/columnar/validator/Validators.scala:1
- The previous implementation used
dataType.typeName == "timestamp_ntz", which is resilient across Spark versions/shims. Switching to a directTimestampNTZTypereference introduces a compile-time dependency that can break builds for Spark variants whereTimestampNTZTypeis absent or shaded differently. If this module is cross-built across multiple Spark versions, consider keeping a version-tolerant check (e.g., matchTimestampNTZTypewhen available and fall back totypeName), ideally via a shim utility.
/*
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive and well-covered by targeted Spark + native regression tests, and the updated validator/signature/type mappings are consistent across JVM and C++ paths.
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Lite
@rui-mo facebookincubator/velox#18963. I left it in draft, I'm not sure if the tests are ok: 
|
d3f1118 to
ef142e5
Compare
|
Run Gluten Clickhouse CI on x86 |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Observe the task-local statistics plan in tests and require native aggregate offload in both Delta variants. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ef142e5 to
295d31b
Compare
|
Run Gluten Clickhouse CI on x86 |
There was a problem hiding this comment.
🔵 Needs a closer look
Add grouped NTZ aggregate coverage and update the public configuration documentation.
Review details
Suppressed comments (2)
gluten-substrait/src/main/scala/org/apache/gluten/execution/HashAggregateExecBaseTransformer.scala:101
- The new branch makes
TimestampNTZTypevalid as a grouping key, but the added suite only exercises a global aggregate and the Delta statistics plan usesSeq.emptygrouping. Add a grouped NTZ aggregate assertion (including native-plan verification and per-key min/max results) so regressions in the grouping path covered by this change are detected.
case BooleanType | StringType | TimestampType | TimestampNTZType | DateType | BinaryType =>
gluten-substrait/src/main/scala/org/apache/gluten/extension/columnar/validator/Validators.scala:283
- This expands the meaning of
enableTimestampNtzValidation=falsebeyond the scan-only behavior documented byVeloxConfig.ENABLE_TIMESTAMP_NTZ_VALIDATION(which currently says it allows native TimestampNTZ scans only). Please update that public configuration documentation alongside this validator change so users know that aggregates, shuffles, and the listed projections are also admitted, or split the controls if scan-only behavior is still intended.
val isSupportedNtz = plan match {
case _: HashAggregateExec | _: ObjectHashAggregateExec | _: SortAggregateExec => true
case _: ShuffleExchangeExec => true
- Files reviewed: 15/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
Add grouped min/max coverage with TIMESTAMP_NTZ keys and values. Check exact per-key results and require every aggregate stage to use native execution with an NTZ grouping key. Clarify that disabling TimestampNTZ validation permits supported scans, aggregates, shuffles and projections while other validation rules still apply. Regenerate the public configuration reference. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed both suggestions from this review in b97a45a9d2. Added a grouped min/max regression with a Updated the |
|
Run Gluten Clickhouse CI on x86 |
rui-mo
left a comment
rui-mo
left a comment
There was a problem hiding this comment.
This PR looks good to me overall. I just left a few nits and approved the non-Delta changes. I’ll defer to @zhztheplayer on the Delta-related parts.
| "When false, supported TimestampNTZ scans, aggregates, shuffles and projections " + | ||
| "are eligible for native execution. Supported projections include direct columns " + | ||
| "and struct fields, casts, hour/minute/second extraction, timestamp addition " + | ||
| "and null checks. Other operator, expression and native validation rules still apply.") |
There was a problem hiding this comment.
Supported projections include direct columns and struct fields, casts, hour/minute/second extraction, timestamp addition and null checks.
Support for timestamp_ntz is evolving quickly and may change frequently, so could we avoid documenting those implementation details for now? It would be better to keep the configuration documentation at a high level.
| | spark.gluten.sql.columnar.backend.velox.directorySizeGuess | ⚓ Static | 32KB | Deprecated, rename to spark.gluten.sql.columnar.backend.velox.footerEstimatedSize | | ||
| | spark.gluten.sql.columnar.backend.velox.driverSideBroadcastHashTableBuild | 🔄 Dynamic | false | Enable driver-side broadcast hash table build. When enabled, the hash table is built and serialized on the driver, then broadcast to executors. When disabled, each executor builds its own hash table from the broadcast data. | | ||
| | spark.gluten.sql.columnar.backend.velox.enableTimestampNtzValidation | 🔄 Dynamic | false | Enable validation fallback for TimestampNTZ type. When true, any plan containing TimestampNTZ will fall back to Spark execution. When false, allows native execution for TimestampNTZ scan. | | ||
| | spark.gluten.sql.columnar.backend.velox.enableTimestampNtzValidation | 🔄 Dynamic | false | Enable validation fallback for TimestampNTZ type. When true, plans with TimestampNTZ in their input or output schemas fall back to Spark execution. When false, supported TimestampNTZ scans, aggregates, shuffles and projections are eligible for native execution. Supported projections include direct columns and struct fields, casts, hour/minute/second extraction, timestamp addition and null checks. Other operator, expression and native validation rules still apply. | |
| case Second(child, _) => containsNTZ(child.dataType) | ||
| case TimestampAdd(_, _, child, _) => containsNTZ(child.dataType) | ||
| case c: Cast if isNTZ(c.dataType) || isNTZ(c.child.dataType) => true | ||
| case IsNull(child) => containsNTZ(child.dataType) |
There was a problem hiding this comment.
isnull is also unverified for TIMESTAMP_UTC type in Velox.
What changes are proposed in this pull request?
Enable native
min/maxaggregation for SparkTIMESTAMP_NTZin the Velox backend, including the task-localSortAggregateExecused to collect Delta write statistics.The implementation reuses Velox's existing Spark timestamp min/max kernels rather than introducing new aggregate kernels.
TIMESTAMP_NTZremains represented by the distinct VeloxTIMESTAMP_UTClogical type, with microsecond precision (Velox implementation, Spark registration).TimestampNTZTypein aggregate buffer/result and grouping type checks, and allow aggregate, shuffle, and the projection expressions needed by Delta statistics through the coarse NTZ fallback validator. Existing native function validation remains in place.tsntzas the internal function-signature token. The previousts_ntztoken collides with_, which the native signature parser uses to separate argument types (parser).TIMESTAMP_UTCas SubstraitPrecisionTimestampwith precision6during type round-tripping.This addresses the NTZ aggregation failure encountered by the Delta statistics tracker in #12538. It does not implement a general fallback for arbitrary unsupported statistics plans or claim support for every NTZ expression. DATE/TIMESTAMP_NTZ cast support is handled separately in #12966.
Related #11622
How was this patch tested?
6, and preservation of microsecond values using Spark min/max.GlutenTimestampNtzAggregateSuiteon Spark 4.1 checks native SQL min/max execution and microsecond results, plus Spark fallback for NTZ JSON serialization in a non-UTC session timezone.GlutenDeltaStatsSuiteon Spark 3.5/Delta 3.3 and Spark 4.1/Delta 4.x checks writes and read-back of top-level and nested NTZ columns, with exact JSON-path assertions for their min/max statistics.DateFunctionsValidateSuitecoverage exercises NTZ scalar functions after the signature-token change.Was this patch authored or co-authored using generative AI tooling?
Generated-by: GitHub Copilot CLI 1.0.83