Skip to content

[GLUTEN-12538][VL] Support TIMESTAMP_NTZ aggregation - #12967

Open
felipepessoto wants to merge 8 commits into
apache:mainfrom
felipepessoto:gluten-12538-timestamp-ntz-sort-aggregate
Open

[GLUTEN-12538][VL] Support TIMESTAMP_NTZ aggregation#12967
felipepessoto wants to merge 8 commits into
apache:mainfrom
felipepessoto:gluten-12538-timestamp-ntz-sort-aggregate

Conversation

@felipepessoto

@felipepessotofelipepessoto commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

What changes are proposed in this pull request?

Enable native TIMESTAMP_NTZ aggregation in the Velox backend:

  • Allow aggregate and shuffle operators, plus the projection expressions used by Delta statistics collection, through the TIMESTAMP_NTZ fallback validator.
  • Accept TimestampNTZType aggregate buffers.
  • Preserve TIMESTAMP_NTZ as a distinct tsntz function-signature type and as Substrait PrecisionTimestamp.
  • Add native min/max and Delta statistics regression coverage, including nested TIMESTAMP_NTZ columns.
  • Remove 42 Delta data-skipping tests fixed by the native statistics aggregation support from the known-failure baseline.

This prevents Delta's local statistics SortAggregateExec from falling back to a vanilla Spark plan and addresses the TIMESTAMP_NTZ failure described in #12538. This change is independent of #12966.

How was this patch tested?

  • cpp/build/velox/tests/velox_plan_conversion_test --gtest_filter='VeloxSubstraitSignatureTest.*:VeloxToSubstraitTypeTest.*:VeloxSubstraitRoundTripTest.minMaxTimestampUtc'
  • ./build/mvn test -Pspark-ut -Pbackends-velox -Pspark-4.1 -Pscala-2.13 -Pjava-17 -Pdelta -DargLine="-Dspark.test.home=/opt/shims/spark41/spark_home/" -DwildcardSuites=org.apache.spark.sql.GlutenTimestampNtzAggregateSuite
  • ./build/mvn test -Pspark-ut -Pbackends-velox -Pspark-4.1 -Pscala-2.13 -Pjava-17 -Pdelta -DargLine="-Dspark.test.home=/opt/shims/spark41/spark_home/" -DwildcardSuites=org.apache.spark.sql.delta.GlutenDeltaStatsSuite
  • ./build/mvn test -Pspark-ut -Pbackends-velox -Pspark-3.5 -Pscala-2.12 -Pjava-17 -Pdelta -DargLine="-Dspark.test.home=/opt/shims/spark35/spark_home/" -DwildcardSuites=org.apache.spark.sql.delta.GlutenDeltaStatsSuite
  • ./build/mvn test -Pspark-ut -Pbackends-velox -Pspark-3.5 -Pscala-2.12 -Pjava-17 -Pdelta -DargLine="-Dspark.test.home=/opt/shims/spark35/spark_home/" -DwildcardSuites=org.apache.gluten.functions.DateFunctionsValidateSuite

Was this patch authored or co-authored using generative AI tooling?

Generated-by: GitHub Copilot CLI 1.0.82

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actionsgithub-actionsBot added CORE works for Gluten Core VELOX labels Sep 4, 2026
@github-actions

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

@github-actions

Copy link
Copy Markdown

🔄 Delta Spark UT started by @felipepessoto (~2.5 h). View run

@github-actions

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

@felipepessoto
felipepessoto marked this pull request as ready for review September 4, 2026 09:33
CopilotAI lite review requested due to automatic review settings September 4, 2026 09:33

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 TimestampNTZ fallback validator to allow aggregates, shuffles, and direct NTZ projections needed by Delta stats plans.
  • Accept TimestampNTZType in aggregation buffer type checks and preserve a distinct native signature token (tsntz) plus Substrait PrecisionTimestamp encoding.
  • Add native regression coverage (Spark UT + Velox C++ tests) and remove now-fixed Delta data-skipping cases from the known-failure baseline.
File summaries
FileDescription
gluten-ut/spark41/src/test/scala/org/apache/spark/sql/GlutenTimestampNtzAggregateSuite.scalaAdds 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.scalaEnables the new Spark 4.1 UT suite in Velox test settings.
gluten-substrait/src/main/scala/org/apache/gluten/extension/columnar/validator/Validators.scalaBroadens NTZ fallback validation to permit additional plan nodes/expressions used by native stats aggregation.
gluten-substrait/src/main/scala/org/apache/gluten/expression/ConverterUtils.scalaMaps Spark TimestampNTZType to Substrait timestamp NTZ type node and uses tsntz in signature naming.
gluten-substrait/src/main/scala/org/apache/gluten/execution/HashAggregateExecBaseTransformer.scalaAllows TimestampNTZType in supported aggregation buffer type checks.
cpp/velox/tests/VeloxToSubstraitTypeTest.ccAdds a type-conversion test for TIMESTAMP_UTC -> Substrait PrecisionTimestamp.
cpp/velox/tests/VeloxSubstraitSignatureTest.ccAdds tsntz signature coverage for TIMESTAMP_UTC mapping in both directions.
cpp/velox/tests/VeloxSubstraitRoundTripTest.ccAdds a min/max aggregation round-trip test over TIMESTAMP_UTC.
cpp/velox/substrait/VeloxToSubstraitType.ccEncodes TIMESTAMP_UTC as Substrait precision_timestamp(6).
cpp/velox/substrait/VeloxSubstraitSignature.ccMaps TIMESTAMP_UTC to/from the tsntz signature token.
backends-velox/src-delta40/test/scala/org/apache/spark/sql/delta/GlutenDeltaStatsSuite.scalaAdds Delta 4.0 stats regression coverage including nested NTZ columns.
backends-velox/src-delta33/test/scala/org/apache/spark/sql/delta/GlutenDeltaStatsSuite.scalaAdds Delta 3.3 stats regression coverage including nested NTZ columns.
.github/workflows/util/delta-spark-ut/known-failures.txtRemoves 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.

Comment threadcpp/velox/tests/VeloxSubstraitRoundTripTest.cc Outdated
@felipepessoto

Copy link
Copy Markdown
ContributorAuthor

@Mariamalmesfer@rui-mo I think you two have worked on other timestamp_ntz PRs. Could you take a look, please?

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings September 4, 2026 17:57

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment threadcpp/velox/tests/VeloxSubstraitRoundTripTest.cc Outdated
Comment threadcpp/velox/substrait/VeloxToSubstraitType.cc Outdated
@github-actions

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings September 4, 2026 19:06
@github-actions

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 direct TimestampNTZType reference introduces a compile-time dependency that can break builds for Spark variants where TimestampNTZType is absent or shaded differently. If this module is cross-built across multiple Spark versions, consider keeping a version-tolerant check (e.g., match TimestampNTZType when available and fall back to typeName), ideally via a shim utility.
/*

Comment threadcpp/velox/substrait/VeloxSubstraitSignature.cc
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings September 4, 2026 20:13
@github-actions

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

@github-actions

Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

COREworks for Gluten CoreINFRAVELOX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@felipepessoto