You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "snuyanzin (via GitHub)" <gi...@apache.org> on 2025/05/18 14:10:54 UTC
[PR] [CALCITE-7026] Validation of snapshot for join of CTEs fails with NPE [calcite]
snuyanzin opened a new pull request, #4388:
URL: https://github.com/apache/calcite/pull/4388
(no comment)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot for join of CTEs fails with NPE [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #4388:
URL: https://github.com/apache/calcite/pull/4388#discussion_r2097272942
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -5874,6 +5874,11 @@ private void validateSnapshot(
throw newValidationError(period,
Static.RESOURCE.illegalExpressionForTemporal(dataType.getSqlTypeName().getName()));
}
+ if (ns instanceof IdentifierNamespace && ns.resolve() instanceof WithItemNamespace) {
+ // If the snapshot is used over a CTE, then we don't have a concrete underlying
+ // table to operate on. This will be rechecked later in the planner rules.
Review Comment:
in case of Flink it depends... It might be defined per query however then there is extra requirement of having primary key which is checked during planning.
It seems the behavior might be depending on downstream projects.
Then I'm thinking of having a `protected` method here which is failing by default in case of queries and downstream projects could override it if needed
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot in case of CTEs should fail with better message than NPE [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4388: [CALCITE-7026] Validation of snapshot in case of CTEs should fail with better message than NPE
URL: https://github.com/apache/calcite/pull/4388
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot for join of CTEs fails with NPE [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4388:
URL: https://github.com/apache/calcite/pull/4388#discussion_r2094671067
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -5874,6 +5874,11 @@ private void validateSnapshot(
throw newValidationError(period,
Static.RESOURCE.illegalExpressionForTemporal(dataType.getSqlTypeName().getName()));
}
+ if (ns instanceof IdentifierNamespace && ns.resolve() instanceof WithItemNamespace) {
+ // If the snapshot is used over a CTE, then we don't have a concrete underlying
+ // table to operate on. This will be rechecked later in the planner rules.
Review Comment:
is SYSTEM_TIME even defined for a query?
Or just for a table?
If not, maybe the query should be rejected.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot for join of CTEs fails with NPE [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4388:
URL: https://github.com/apache/calcite/pull/4388#discussion_r2094584188
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -5874,6 +5874,11 @@ private void validateSnapshot(
throw newValidationError(period,
Static.RESOURCE.illegalExpressionForTemporal(dataType.getSqlTypeName().getName()));
}
+ if (ns instanceof IdentifierNamespace && ns.resolve() instanceof WithItemNamespace) {
+ // If the snapshot is used over a CTE, then we don't have a concrete underlying
+ // table to operate on. This will be rechecked later in the planner rules.
Review Comment:
there is no guarantee that any planner rules will be executed after validation.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot in case of CTEs should fail with better message than NPE [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4388:
URL: https://github.com/apache/calcite/pull/4388#issuecomment-3093179537
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot for join of CTEs fails with NPE [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #4388:
URL: https://github.com/apache/calcite/pull/4388#discussion_r2094588326
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -5874,6 +5874,11 @@ private void validateSnapshot(
throw newValidationError(period,
Static.RESOURCE.illegalExpressionForTemporal(dataType.getSqlTypeName().getName()));
}
+ if (ns instanceof IdentifierNamespace && ns.resolve() instanceof WithItemNamespace) {
+ // If the snapshot is used over a CTE, then we don't have a concrete underlying
+ // table to operate on. This will be rechecked later in the planner rules.
Review Comment:
i think in general you are right
I backported it from Flink where it is guaranteed...
Any proposal how to proceed with Calcite would be great to hear
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot in case of CTEs should fail with better message than NPE [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #4388:
URL: https://github.com/apache/calcite/pull/4388#discussion_r2114666513
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -8782,6 +8782,12 @@ void testGroupExpressionEquivalenceParams() {
sql("select stream * from orders left join products_temporal\n"
+ "for system_time as of CURRENT_TIMESTAMP\n"
+ "on orders.productid = products_temporal.productid").ok();
+
+ // verify left join of CTEs
Review Comment:
made it failing and overridable
>but is there even a definition of what it means to apply SYSTEM_TIME to the result of a query?
in Flink result of query in streaming mode is changelog. Changelog has timestamp, in this case we could jump at some position in the middle with SYSTEM_TIME
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot in case of CTEs should fail with better message than NPE [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4388:
URL: https://github.com/apache/calcite/pull/4388#issuecomment-2920489853
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4388) **Quality Gate passed**
Issues
 [5 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4388&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4388&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4388&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [88.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4388&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4388&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4388)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot in case of CTEs should fail with better message than NPE [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4388:
URL: https://github.com/apache/calcite/pull/4388#issuecomment-3882036808
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot for join of CTEs fails with NPE [calcite]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #4388:
URL: https://github.com/apache/calcite/pull/4388#discussion_r2094588326
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -5874,6 +5874,11 @@ private void validateSnapshot(
throw newValidationError(period,
Static.RESOURCE.illegalExpressionForTemporal(dataType.getSqlTypeName().getName()));
}
+ if (ns instanceof IdentifierNamespace && ns.resolve() instanceof WithItemNamespace) {
+ // If the snapshot is used over a CTE, then we don't have a concrete underlying
+ // table to operate on. This will be rechecked later in the planner rules.
Review Comment:
i think in general you are right
I backported it from Flink where it is guaranteed...
Any proposal how to proceed with Calcite
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot for join of CTEs fails with NPE [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4388:
URL: https://github.com/apache/calcite/pull/4388#discussion_r2098598102
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -8782,6 +8782,12 @@ void testGroupExpressionEquivalenceParams() {
sql("select stream * from orders left join products_temporal\n"
+ "for system_time as of CURRENT_TIMESTAMP\n"
+ "on orders.productid = products_temporal.productid").ok();
+
+ // verify left join of CTEs
Review Comment:
I think that this query is illegal according to the "standard" definition of SYSTEM_TIME.
You can make the method overridable, but is there even a definition of what it means to apply SYSTEM_TIME to the result of a query?
If you want it in here it should be applied to products_temporal.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot in case of CTEs should fail with better message than NPE [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4388:
URL: https://github.com/apache/calcite/pull/4388#discussion_r2157662451
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -8782,6 +8782,12 @@ void testGroupExpressionEquivalenceParams() {
sql("select stream * from orders left join products_temporal\n"
+ "for system_time as of CURRENT_TIMESTAMP\n"
+ "on orders.productid = products_temporal.productid").ok();
+
+ // verify left join of CTEs
Review Comment:
One way this can be resolved is to have a conformance flag (systemTimeForQueries?) which makes this query legal, and then disable this check only if that conformance flag is enabled. Supposedly compiling for Flink would enable it.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot for join of CTEs fails with NPE [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4388:
URL: https://github.com/apache/calcite/pull/4388#issuecomment-2889028970
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4388) **Quality Gate passed**
Issues
 [5 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4388&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4388&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4388&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [83.3% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4388&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4388&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4388)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot in case of CTEs should fail with better message than NPE [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4388:
URL: https://github.com/apache/calcite/pull/4388#issuecomment-3445639811
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7026] Validation of snapshot in case of CTEs should fail with better message than NPE [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4388:
URL: https://github.com/apache/calcite/pull/4388#issuecomment-4427411095
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org