test(integ-test): isolate invalid datetime evaluation from scans - #5745
Open
mengweieric wants to merge 1 commit into
Open
test(integ-test): isolate invalid datetime evaluation from scans#5745mengweieric wants to merge 1 commit into
mengweieric wants to merge 1 commit into
Conversation
CalcitePPLBuiltinDatetimeFunctionInvalidIT verifies that invalid datetime literals fail at runtime with the expected exception class and message. Its queries do not reference indexed fields, but each one currently scans a one-document index solely to trigger expression evaluation. Use `makeresults count=1` as the common source instead. This still produces one runtime row and preserves all 146 invalid-function assertions, while removing unrelated index, shard, and point-in-time lifecycle behavior from the test class. Remove the fixture loads that are no longer used. The separate Calcite PIT cleanup gap remains a production concern and is not addressed by this test-only change. Signed-off-by: Eric Wei <menwe@amazon.com>
Contributor
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
mengweieric
marked this pull request as ready for review
September 3, 2026 01:36
mengweieric
requested review from
LantaoJin,
RyanL1997,
Swiddis,
acarbonetto,
ahkcs,
anirudha,
dai-chen,
joshuali925,
noCharger,
penghuo,
ps48,
qianheng-aws,
songkant-aws,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
September 3, 2026 01:36
dai-chen
approved these changes
Sep 3, 2026
dai-chen
left a comment
dai-chen
left a comment
Collaborator
There was a problem hiding this comment.
just curious why PIT was triggered by each test in this IT?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
CalcitePPLBuiltinDatetimeFunctionInvalidITverifies that invalid datetime literals fail at runtime with the expected exception class and message. The class contains 146 assertions, but none of its queries reference an indexed field. The existing one-document index is used only to produce a row for expression evaluation.Scanning that index also brings shard and point-in-time lifecycle behavior into tests whose goal is limited to function validation. With multiple shards, repeated failing scans can exhaust the PIT context limit before the remaining datetime assertions run.
This change uses
makeresults count=1as the common source for every query. It still produces one row and evaluates the same invalid literals at runtime, while removing the unrelated index scan. The four fixture loads are no longer needed and are removed.All exception types and message assertions remain unchanged. This is a test-only scope correction; it does not change production behavior or address the separate PIT cleanup path.
Validation
makeresults count=1loadIndexcall remains in the classmakeresults count=0makestestYearInvalidfail in both modes because no row evaluates the expressionspotlessJavaCheck,compileTestJava, andgit diff --checkpassThe large line-count diff is mechanical: the same source substitution is applied to all 146 assertions, and Spotless collapses the now-shorter
String.formatexpressions.Related Issues
None.
Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.