Skip to content

[DATA-2616] Disallow non-deterministic ingestion transforms#18932

Open
xiangfu0 wants to merge 1 commit into
masterfrom
data-2616-disallow-nondeterministic-transforms
Open

[DATA-2616] Disallow non-deterministic ingestion transforms#18932
xiangfu0 wants to merge 1 commit into
masterfrom
data-2616-disallow-nondeterministic-transforms

Conversation

@xiangfu0

@xiangfu0 xiangfu0 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • reject non-deterministic scalar functions when validating or constructing ingestion transform evaluators
  • mark now() and ago()/agoMV() as non-deterministic so now()/ago() transforms fail fast
  • keep default expression evaluator behavior permissive for existing non-ingestion callers

Tests

  • GITHUB_ACTIONS=true ./mvnw -pl pinot-common -am -Dtest=InbuiltFunctionEvaluatorTest,CalciteSqlCompilerTest -Dsurefire.failIfNoSpecifiedTests=false test
  • GITHUB_ACTIONS=true ./mvnw -pl pinot-segment-local -am -Dtest=ExpressionTransformerTest,TableConfigUtilsTest,SchemaUtilsTest -Dsurefire.failIfNoSpecifiedTests=false test
  • GITHUB_ACTIONS=true ./mvnw -pl pinot-core -am -Dtest=FunctionRegistryTest -Dsurefire.failIfNoSpecifiedTests=false test
  • ./mvnw spotless:apply -pl pinot-common,pinot-segment-local,pinot-core
  • GITHUB_ACTIONS=true ./mvnw checkstyle:check -pl pinot-common,pinot-segment-local,pinot-core
  • ./mvnw license:format -pl pinot-common,pinot-segment-local,pinot-core
  • GITHUB_ACTIONS=true ./mvnw license:check -pl pinot-common,pinot-segment-local,pinot-core
  • git diff --check

@xiangfu0 xiangfu0 added bug Something is not working as expected ingestion Related to data ingestion pipeline functions Related to scalar or aggregation functions labels Jul 8, 2026
@xiangfu0 xiangfu0 requested review from Jackie-Jiang and Copilot July 8, 2026 05:37
@xiangfu0 xiangfu0 requested a review from deepthi912 July 8, 2026 05:38

Copilot AI 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.

Pull request overview

This PR tightens ingestion-time transform validation by rejecting non-deterministic scalar functions (e.g., now(), ago(), rand() without seed) when building/validating ingestion transform evaluators, while keeping the existing default (non-ingestion) expression evaluator behavior permissive. It also marks now()/ago()/agoMV() as non-deterministic so they are no longer eligible for compile-time evaluation and will fail fast in ingestion contexts.

Changes:

  • Add a “deterministic-only” evaluator mode (FunctionEvaluatorFactory.getDeterministicExpressionEvaluator(...)) and enforce it for ingestion transforms (table ingestion transforms, schema transform functions during ingestion, default-column derived column creation, and expression transformer construction).
  • Mark time-based functions (now(), ago(), agoMV()) as non-deterministic via @ScalarFunction(isDeterministic = false), and rely on determinism metadata (FunctionInfo#isDeterministic) for both compile-time folding and deterministic-only evaluation.
  • Expand/adjust unit tests to validate determinism behavior and updated SQL compilation outcomes.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/TableConfigUtilsTest.java Adds assertions that ingestion and post-partial-upsert transforms reject non-deterministic functions.
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/utils/SchemaUtilsTest.java Adds schema validation tests ensuring non-deterministic schema transform functions are rejected.
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/ExpressionTransformerTest.java Verifies ExpressionTransformer construction fails fast for non-deterministic transform functions.
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java Switches ingestion transform validation to deterministic-only expression evaluator.
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/SchemaUtils.java Uses deterministic-only evaluator when validating schema transform functions’ arguments.
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/defaultcolumn/BaseDefaultColumnHandler.java Ensures derived-column transforms used during segment loading are deterministic-only.
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/ExpressionTransformer.java Builds ingestion transform evaluators using deterministic-only evaluation.
pinot-core/src/test/java/org/apache/pinot/core/function/FunctionRegistryTest.java Adds determinism assertions for now, ago, and rand overloads.
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java Updates SQL compiler expectations so non-deterministic functions are not folded into literals.
pinot-common/src/test/java/org/apache/pinot/common/evaluator/InbuiltFunctionEvaluatorTest.java Tests deterministic-only mode rejection of non-deterministic functions (including nested usage).
pinot-common/src/main/java/org/apache/pinot/common/function/scalar/DateTimeFunctions.java Marks now, ago, agoMV as non-deterministic via annotation metadata.
pinot-common/src/main/java/org/apache/pinot/common/evaluator/InbuiltFunctionEvaluator.java Adds deterministic-only enforcement using FunctionInfo.isDeterministic().
pinot-common/src/main/java/org/apache/pinot/common/evaluator/FunctionEvaluatorFactory.java Adds deterministic-only factory methods and threads the “allowNonDeterministic” flag into evaluator construction.

@xiangfu0 xiangfu0 force-pushed the data-2616-disallow-nondeterministic-transforms branch 3 times, most recently from 6a548a3 to e35c9d9 Compare July 9, 2026 00:12

@Jackie-Jiang Jackie-Jiang 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.

This will break existing tables right?
Should we just reject non-deterministic functions during table config validation?

@codecov-commenter

codecov-commenter commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.30769% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.99%. Comparing base (e7b57c0) to head (65a9fd1).

Files with missing lines Patch % Lines
...he/pinot/segment/local/utils/TableConfigUtils.java 91.30% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master   #18932   +/-   ##
=========================================
  Coverage     64.98%   64.99%           
+ Complexity     1398     1392    -6     
=========================================
  Files          3399     3399           
  Lines        212770   212815   +45     
  Branches      33558    33566    +8     
=========================================
+ Hits         138274   138322   +48     
+ Misses        63363    63357    -6     
- Partials      11133    11136    +3     
Flag Coverage Δ
custom-integration1 ?
integration ?
integration1 ?
integration2 ?
java-21 64.99% <92.30%> (+<0.01%) ⬆️
temurin 64.99% <92.30%> (+<0.01%) ⬆️
unittests 64.99% <92.30%> (+0.01%) ⬆️
unittests1 56.99% <47.82%> (+0.02%) ⬆️
unittests2 37.33% <92.30%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xiangfu0 xiangfu0 force-pushed the data-2616-disallow-nondeterministic-transforms branch from e35c9d9 to c8da017 Compare July 9, 2026 01:15

@xiangfu0 xiangfu0 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Found one high-confidence compatibility issue; see inline comment.

@xiangfu0 xiangfu0 force-pushed the data-2616-disallow-nondeterministic-transforms branch from c8da017 to 0485e90 Compare July 9, 2026 03:31
@xiangfu0

xiangfu0 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Updated per review: the PR now rejects non-deterministic functions only for new or changed ingestion transformConfigs and postPartialUpsertTransformConfigs during table config validation. Unchanged legacy transforms are allowed during update validation, and runtime ExpressionTransformer remains permissive, so existing tables keep ingesting and can receive unrelated config edits.

I also removed the FunctionEvaluatorFactory#getDeterministicExpressionEvaluator API change; the deterministic check is private to table validation and uses functionInfo.isDeterministic() for annotated functions like unseeded rand(), plus a validation-only request-time check for now()/ago()/agoMV.

For the runtime compatibility test feedback, I renamed the test to make the legacy-compatibility scope explicit and added a 1s wall-clock tolerance.

@xiangfu0 xiangfu0 force-pushed the data-2616-disallow-nondeterministic-transforms branch from 0485e90 to e3f611d Compare July 9, 2026 04:57

@xiangfu0 xiangfu0 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Found one high-confidence compatibility issue; see inline comment.

Copilot AI 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.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:732

  • getDeterministicExpressionEvaluator() can throw for both syntactically invalid expressions and for disallowed non-deterministic functions. Wrapping both cases under "Invalid transform function" is misleading for the non-deterministic rejection path and makes it harder to understand why a previously-valid expression is now rejected.
          } catch (Exception e) {
            throw new IllegalStateException(
                "Invalid transform function '" + transformFunction + "' for column '" + columnName + "', exception: "
                    + e.getMessage(), e);

@xiangfu0 xiangfu0 force-pushed the data-2616-disallow-nondeterministic-transforms branch 7 times, most recently from 79816a2 to 7bc1145 Compare July 10, 2026 23:24
Reject non-deterministic built-in functions during table config validation for new or changed ingestion transform configs and post-partial-upsert transform configs.

Keep runtime expression evaluation permissive, and allow unchanged legacy transform configs during table updates so existing tables continue ingesting and can receive unrelated config edits.
@xiangfu0 xiangfu0 force-pushed the data-2616-disallow-nondeterministic-transforms branch from 7bc1145 to 65a9fd1 Compare July 11, 2026 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something is not working as expected functions Related to scalar or aggregation functions ingestion Related to data ingestion pipeline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants