[GitHub] [calcite] sauajmera commented on pull request #2984: Rtp 203_to_char_with_trunc_results_in_datatype_mismatch
sauajmera commented on PR #2984: URL: https://github.com/apache/calcite/pull/2984#issuecomment-1328948602 Wrong branch -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] sauajmera closed pull request #2984: Rtp 203_to_char_with_trunc_results_in_datatype_mismatch
sauajmera closed pull request #2984: Rtp 203_to_char_with_trunc_results_in_datatype_mismatch URL: https://github.com/apache/calcite/pull/2984 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
libenchao commented on code in PR #2977: URL: https://github.com/apache/calcite/pull/2977#discussion_r1033044521 ## core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java: ## @@ -693,7 +696,7 @@ private static Pair getPartitionIterator( } Expression multiMap_ = builder.append( -"multiMap", Expressions.new_(SortedMultiMap.class)); +"multiMap", Expressions.new_(SortedMultiMap.class), false); Review Comment: Thanks for your detailed explanation. I knew it's unsafe, what I propose it to have a test case to reproduce it and cover it if we can. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
dssysolyatin commented on code in PR #2977:
URL: https://github.com/apache/calcite/pull/2977#discussion_r1033016387
##
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java:
##
@@ -693,7 +696,7 @@ private static Pair
getPartitionIterator(
}
Expression multiMap_ =
builder.append(
-"multiMap", Expressions.new_(SortedMultiMap.class));
+"multiMap", Expressions.new_(SortedMultiMap.class), false);
Review Comment:
multiMap works OK at the moment even with optimize = true. But it is very
unsafe. I will try to explain why. The different between multiMap and tempList
cases are following.
Code for tempList case looks like:
```
final java.util.List tempList = (java.util.List)
org.apache.calcite.linq4j.Linq4j.asEnumerable(new Integer[] {1,2})
.into(new java.util.ArrayList()); <--this expression will be
reused
tempList.clear()
<-- Second EnumerableWindow will use the same tempList and will not fill
tempList again -->
```
Code for multiMap looks like:
```
final org.apache.calcite.runtime.SortedMultiMap multiMap =
new org.apache.calcite.runtime.SortedMultiMap(); <--this expression
will be reused
org.apache.calcite.linq4j.Linq4j.asEnumerable(new Integer[]
{1,2}).foreach()
multiMap.clear()
<-- Second EnumerableWindow will use the same multiMap but also will refill
it -->
```
Code works OK for multiMap only because the creation and filling of the
multiMap are in different expressions. But it is very unsafe. If for some
reason creating and filling multiMap are in the same expression code will also
stop working.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
dssysolyatin commented on code in PR #2977:
URL: https://github.com/apache/calcite/pull/2977#discussion_r1033016387
##
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java:
##
@@ -693,7 +696,7 @@ private static Pair
getPartitionIterator(
}
Expression multiMap_ =
builder.append(
-"multiMap", Expressions.new_(SortedMultiMap.class));
+"multiMap", Expressions.new_(SortedMultiMap.class), false);
Review Comment:
multiMap works OK at the moment even with optimize = true. But it is very
unsafe. I will try to explain why. The different between multiMap and tempList
cases are following.
Code for tempList case looks like:
```
final java.util.List tempList = (java.util.List)
org.apache.calcite.linq4j.Linq4j.asEnumerable(new Integer[] {1,2})
.into(new java.util.ArrayList()); <--this expression will be
reused
tempList.clear()
<-- Second EnumerableWindow will use the same tempList and will not fill
tempList again -->
```
Code for multiMap looks like:
```
final org.apache.calcite.runtime.SortedMultiMap multiMap =
new org.apache.calcite.runtime.SortedMultiMap(); <--this expression
will be reused
org.apache.calcite.linq4j.Linq4j.asEnumerable(new Integer[]
{1,2}).foreach()
multiMap.clear()
<-- Second EnumerableWindow will refill multiMap -->
```
Code works OK for multiMap only because the creation and filling of the
multiMap are in different expressions. But it is very unsafe. If for some
reason creating and filling multiMap are in the same expression code will also
stop working.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
dssysolyatin commented on code in PR #2977:
URL: https://github.com/apache/calcite/pull/2977#discussion_r1033016387
##
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java:
##
@@ -693,7 +696,7 @@ private static Pair
getPartitionIterator(
}
Expression multiMap_ =
builder.append(
-"multiMap", Expressions.new_(SortedMultiMap.class));
+"multiMap", Expressions.new_(SortedMultiMap.class), false);
Review Comment:
multiMap works OK at the moment even with optimize = true. But it is very
unsafe. I will try to explain why. The different between multiMap and tempList
cases are following.
Code for tempList case looks like:
```
final java.util.List tempList = (java.util.List)
org.apache.calcite.linq4j.Linq4j.asEnumerable(new Integer[] {1,2})
.into(new
java.util.ArrayList()); <-- this expression will be reused.
tempList.clear()
<> Second EnumerableWindow will use the same tempList and will not fill
tempList again
```
Code for multiMap looks like:
```
final org.apache.calcite.runtime.SortedMultiMap multiMap = new
org.apache.calcite.runtime.SortedMultiMap(); <--this expression will be reused
org.apache.calcite.linq4j.Linq4j.asEnumerable(new Integer[]
{1,2}).foreach()
multiMap.clear()
<> Second EnumerableWindow will refill
```
Code works OK for multiMap only because the creation and filling of the
multiMap are in different expressions. But it is very unsafe. If for some
reason creating and filling multiMap are in the same expression code will also
stop working.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] libenchao merged pull request #2975: [CALCITE-5391] JoinOnUniqueToSemiJoinRule should preserve field names, if possible
libenchao merged PR #2975: URL: https://github.com/apache/calcite/pull/2975 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
libenchao commented on code in PR #2977: URL: https://github.com/apache/calcite/pull/2977#discussion_r1032795142 ## core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java: ## @@ -693,7 +696,7 @@ private static Pair getPartitionIterator( } Expression multiMap_ = builder.append( -"multiMap", Expressions.new_(SortedMultiMap.class)); +"multiMap", Expressions.new_(SortedMultiMap.class), false); Review Comment: Current test case would success without this change, it would be good to have a test to cover this change. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] JiajunBernoulli opened a new pull request, #2983: [CALCITE-5395] RelToSql converter fails when SELECT * is under a semi…
JiajunBernoulli opened a new pull request, #2983: URL: https://github.com/apache/calcite/pull/2983 …-join node -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] JiajunBernoulli opened a new pull request, #2982: [CALCITE-5394] RelToSql converter fails when semi-join is under a joi…
JiajunBernoulli opened a new pull request, #2982: URL: https://github.com/apache/calcite/pull/2982 …n node -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] rubenada merged pull request #2967: [CALCITE-5377] RelFieldTrimmer support Sort with dynamic param
rubenada merged PR #2967: URL: https://github.com/apache/calcite/pull/2967 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] zoudan opened a new pull request, #2981: [CALCITE-5283] Add ARG_MIN, ARG_MAX (aka MIN_BY, MAX_BY) aggregate functions
zoudan opened a new pull request, #2981: URL: https://github.com/apache/calcite/pull/2981 Add ARG_MIN, ARG_MAX (aka MIN_BY, MAX_BY) aggregate functions. - **ARG_MAX(value, comp)** Returns *value* for the maximum value of *comp* in the group - **ARG_MIN(value, comp)**Returns *value* for the minimum value of *comp* in the group -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] wnob commented on a diff in pull request #2980: [CALCITE-5389] Add STARTS_WITH() for BIG_QUERY
wnob commented on code in PR #2980:
URL: https://github.com/apache/calcite/pull/2980#discussion_r1029900371
##
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##
@@ -206,6 +206,14 @@ private SqlLibraryOperators() {
.andThen(SqlTypeTransforms.TO_VARYING), null,
OperandTypes.STRING, SqlFunctionCategory.STRING);
+ /** BigQuery's "SUBSTR(value1, value2)" function. */
Review Comment:
typo?
##
babel/src/main/codegen/includes/parserImpls.ftl:
##
@@ -178,6 +199,7 @@ SqlCreate SqlCreateTable(Span s, boolean replace) :
| < DATEPART: "DATEPART" >
| < NEGATE: "!" >
| < TILDE: "~" >
+| < STARTS_WITH: "STARTS_WITH" >
Review Comment:
nit: I believe this should be alphabetically sorted
##
babel/src/test/resources/sql/big-query.iq:
##
@@ -2519,4 +2519,31 @@ SELECT PARSE_TIMESTAMP("%c", "Thu Dec 25 07:30:00 2008")
AS parsed;
!ok
!}
+#
+#
+# STARTS_WITH(value1, value2)
+#
+# Takes two STRING or BYTES values. Returns TRUE if the second value is a
prefix of the first.
+#
+# This function supports specifying collation.
+WITH items AS
+ (SELECT 'foo' as item
Review Comment:
These are all strings, but your function accepts
`OperandTypes.STRING_STRING` which includes bytes objects as well. We should
test those too (I think you need another overload in `SqlFunctions`).
I also think you may need to specify that both arguments must have the same
type, and I'm not sure if `OperandTypes.STRING_STRING` accomplishes that. There
should be a test to make sure you can't call `STARTS_WITH` passing a string and
a bytes together. Double check that this is consistent with BigQuery by running
a sample query in the BQ console, since their documentation isn't 100% clear.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] olivrlee opened a new pull request, #2980: [CALCITE-5389] Add STARTS_WITH() for BIG_QUERY
olivrlee opened a new pull request, #2980: URL: https://github.com/apache/calcite/pull/2980 Adding `STARTS_WITH()` for `BIG_QUERY` -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] asolimando merged pull request #2976: [CALCITE-5392] Support Snapshot in RelMdExpressionLineage
asolimando merged PR #2976: URL: https://github.com/apache/calcite/pull/2976 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on pull request #2976: [CALCITE-5392] Support Snapshot in RelMdExpressionLineage
JiajunBernoulli commented on PR #2976: URL: https://github.com/apache/calcite/pull/2976#issuecomment-1323695852 @asolimando Thank you again, I squashed to a single commit. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] asolimando commented on pull request #2976: [CALCITE-5392] Support Snapshot in RelMdExpressionLineage
asolimando commented on PR #2976: URL: https://github.com/apache/calcite/pull/2976#issuecomment-1323315072 @JiajunBernoulli it's good to me, if you can squash to a single commit I will merge, thanks! -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] julianhyde closed pull request #2961: [CALCITE-5336] Infer constants from predicates with IS NOT DISTINCT FROM operator
julianhyde closed pull request #2961: [CALCITE-5336] Infer constants from predicates with IS NOT DISTINCT FROM operator URL: https://github.com/apache/calcite/pull/2961 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] julianhyde commented on pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
julianhyde commented on PR #2973: URL: https://github.com/apache/calcite/pull/2973#issuecomment-1322750893 The precision issue really is orthogonal, so at best it muddies the waters. See https://issues.apache.org/jira/browse/CALCITE-5266. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] julianhyde commented on pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
julianhyde commented on PR #2973: URL: https://github.com/apache/calcite/pull/2973#issuecomment-1322748744 Let's move design discussions to the Jira case. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] wnob commented on pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
wnob commented on PR #2973: URL: https://github.com/apache/calcite/pull/2973#issuecomment-1322745325 I would also note that both BQ types have microsecond precision, so either way the existing Calcite `TIMESTAMP` is not a perfect match. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] wnob commented on pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
wnob commented on PR #2973: URL: https://github.com/apache/calcite/pull/2973#issuecomment-1322739188 Of course, we could simply re-interpret those bits as `DATETIME`, as I mentioned. We don't need a new internal representation; we could just make a new type that happens to have the same internal representation. UTC is not exactly relevant for a BQ timestamp. Sure, the epoch is 1970-01-01 00:00:00 UTC, but you could also think of it as 1969-01-01 16:00:00 PST. It's just a point in time. On the other hand, a time zone would be very relevant if we decided to represent BQ `DATETIME` as a Calcite `TIMESTAMP`, since we would have to interpret Calcite's offset-from-epoch in a particular time zone, UTC being the obvious choice. Alternatively, we could pick a new internal representation for BQ `DATETIME` -- e.g. as a Java `LocalDateTime`, which is internally represented as clock/calendar parameters with no time zone -- and avoid the implementation detail of converting from offset to clock/calendar parameters in a particular time zone. It doesn't really matter from an engineering standpoint. Basically, we have 2 equally possible options: 1. Map BQ `TIMESTAMP` to Calcite `TIMESTAMP`, and map BQ `DATETIME` to something else (probably a whole new type that's BQ-specific, call it `BIGQUERY_DATETIME`) 2. Map BQ `DATETIME` to Calcite `TIMESTAMP`, and map BQ `TIMESTAMP` to something else (also probably a whole new type that's BQ-specific, call it `BIGQUERY_TIMESTAMP`). I just don't see any advantages to option 1 over 2. Option 1 would cause confusion, as well as requiring a slightly awkward internal representation for `DATETIME` because it requires a time zone to convert from internal representation to semantic representation. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] julianhyde commented on pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
julianhyde commented on PR #2973: URL: https://github.com/apache/calcite/pull/2973#issuecomment-1322709533 No, BigQuery, a timestamp is represented internally as an offset in microseconds since epoch *UTC*. The "UTC" is a crucial difference. Sure, they're both a bunch of bits. But so is a 64 bit IEEE floating point number. The interpretation of those bits is crucial. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] wnob commented on pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
wnob commented on PR #2973: URL: https://github.com/apache/calcite/pull/2973#issuecomment-1322705468 > BQ `DATETIME` = Calcite `TIMESTAMP` I don't see the connection. In Calcite, a timestamp is represented internally as an offset in milliseconds since epoch. In BigQuery, a timestamp is represented internally as an offset in microseconds since epoch. Apart from the difference in resolution, they're essentially the same. Of course, this can easily be re-interpreted as a `DATETIME` by assuming a default time zone, but I struggle to see how a Calcite `TIMESTAMP` is better mapped to a BQ `DATETIME` than to a BQ `TIMESTAMP`. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] tanclary commented on a diff in pull request #2978: [CALCITE-5385] Add BigQuery as support library for implemented functions
tanclary commented on code in PR #2978: URL: https://github.com/apache/calcite/pull/2978#discussion_r1028364707 ## gradle.properties: ## @@ -27,7 +27,7 @@ systemProp.org.gradle.internal.publish.checksums.insecure=true # This is version for Calcite itself # Note: it should not include "-SNAPSHOT" as it is automatically added by build.gradle.kts # Release version can be generated by using -Prelease or -Prc= arguments -calcite.version=1.33.0 +calcite.version=1.33.0-tannerclary Review Comment: Whoops, good catch. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] tjbanghart commented on a diff in pull request #2978: [CALCITE-5385] Add BigQuery as support library for implemented functions
tjbanghart commented on code in PR #2978: URL: https://github.com/apache/calcite/pull/2978#discussion_r1028359636 ## gradle.properties: ## @@ -27,7 +27,7 @@ systemProp.org.gradle.internal.publish.checksums.insecure=true # This is version for Calcite itself # Note: it should not include "-SNAPSHOT" as it is automatically added by build.gradle.kts # Release version can be generated by using -Prelease or -Prc= arguments -calcite.version=1.33.0 +calcite.version=1.33.0-tannerclary Review Comment: Wouldn't want that slipping in! -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] tanclary opened a new pull request, #2978: [Calcite-5385] Add BigQuery as support library for implemented functions
tanclary opened a new pull request, #2978: URL: https://github.com/apache/calcite/pull/2978 Add BigQuery as a supported library for functions that have already been implemented for other libraries. This ticket overrides Calcite-5384 to include other functions that need BQ added as a supported library. Note: This is my first Calcite PR so any feedback on the changes, the formatting of the PR, or any other aspect is highly appreciated. Thank you! List of functions: MD5() SHA1() GREATEST() LEAST() COSH() SINH() TANH() FROM_BASE64() LEFT() LTRIM() REPEAT() REVERSE() RIGHT() RTRIM() SOUNDEX() TRANSLATE() -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2976: [CALCITE-5392] Support Snapshot in RelMdExpressionLineage
asolimando commented on code in PR #2976:
URL: https://github.com/apache/calcite/pull/2976#discussion_r1028128040
##
core/src/main/java/org/apache/calcite/rel/metadata/RelMdExpressionLineage.java:
##
@@ -391,6 +392,14 @@ protected RelMdExpressionLineage() {}
return mq.getExpressionLineage(rel.getInput(), outputExpression);
}
+ /**
+ * Expression lineage from Snapshot.
+ */
Review Comment:
It's better to keep the PR focused on the topic I guess, but thanks for
volunteering
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2976: [CALCITE-5392] Support Snapshot in RelMdExpressionLineage
asolimando commented on code in PR #2976:
URL: https://github.com/apache/calcite/pull/2976#discussion_r1028126808
##
core/src/main/java/org/apache/calcite/rel/metadata/RelMdExpressionLineage.java:
##
@@ -391,6 +392,18 @@ protected RelMdExpressionLineage() {}
return mq.getExpressionLineage(rel.getInput(), outputExpression);
}
+ /**
+ * Expression lineage from Snapshot.
+ * @param rel Relational expression which is Snapshot
+ * @param mq Metadata query
+ * @param outputExpression Expression which need be inferred
+ * @return If the lineage cannot be inferred, we return null.
+ */
Review Comment:
Thanks for adding the javadoc, in general the sentence starts with
lower-case letters, I have taken the liberty to also rephrase a couple of
sentences. No problem if you want to keep the original sentences, but please
fix the first letter casing if you do so.
```suggestion
/**
* Expression lineage from Snapshot.
* @param rel Snapshot relational expression
* @param mq metadata query
* @param outputExpression expression which needs to be inferred
* @return the inferred lineage, possibly null.
*/
```
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] JiajunBernoulli commented on pull request #2976: [CALCITE-5392] Support Snapshot in RelMdExpressionLineage
JiajunBernoulli commented on PR #2976: URL: https://github.com/apache/calcite/pull/2976#issuecomment-1322142351 Sorry, I'll pay attention later. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] asolimando commented on pull request #2976: [CALCITE-5392] Support Snapshot in RelMdExpressionLineage
asolimando commented on PR #2976: URL: https://github.com/apache/calcite/pull/2976#issuecomment-1322134305 @JiajunBernoulli, until the review process is over please don't force-push, it destroys the "link" to the old comments and it makes incremental reviewing impossible (checking just the delta from the latest review). In this case the PR is small-sized, but let's avoid it altogether -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2976: [CALCITE-5392] Support Snapshot in RelMdExpressionLineage
asolimando commented on code in PR #2976:
URL: https://github.com/apache/calcite/pull/2976#discussion_r1027756107
##
core/src/main/java/org/apache/calcite/rel/metadata/RelMdExpressionLineage.java:
##
@@ -391,6 +392,14 @@ protected RelMdExpressionLineage() {}
return mq.getExpressionLineage(rel.getInput(), outputExpression);
}
+ /**
+ * Expression lineage from Snapshot.
+ */
Review Comment:
I know that none of the other methods in the class have it, but I think it's
important to have proper javadoc for public methods of this kind, something
along this format:
```
/**
* Expression lineage from Snapshot.
* @param rel
* @param mq
* @param outputExpression
* @return
*/
```
What do you think Jiajun?
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] julianhyde commented on pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
julianhyde commented on PR #2973: URL: https://github.com/apache/calcite/pull/2973#issuecomment-1320951570 BQ `DATETIME` = Calcite `TIMESTAMP` -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #2848: [CALCITE-5201] Improve SemiJoinRule to match Join's right input which is unique for Join keys
libenchao commented on code in PR #2848:
URL: https://github.com/apache/calcite/pull/2848#discussion_r1027061254
##
core/src/main/java/org/apache/calcite/rel/rules/SemiJoinRule.java:
##
@@ -232,6 +234,84 @@ default JoinToSemiJoinRuleConfig
withOperandFor(Class joinClass,
}
}
+ /**
+ * SemiJoinRule that matches a Project on top of a Join with a RelNode
+ * which is unique for Join's right keys.
+ *
+ * @see CoreRules#JOIN_ON_UNIQUE_TO_SEMI_JOIN */
+ public static class JoinOnUniqueToSemiJoinRule extends SemiJoinRule {
+
+/** Creates a JoinOnUniqueToSemiJoinRule. */
+protected JoinOnUniqueToSemiJoinRule(JoinOnUniqueToSemiJoinRuleConfig
config) {
+ super(config);
+}
+
+@Override public boolean matches(RelOptRuleCall call) {
+ final Project project = call.rel(0);
+ final Join join = call.rel(1);
+ final RelNode left = call.rel(2);
+
+ final ImmutableBitSet bits =
+ RelOptUtil.InputFinder.bits(project.getProjects(), null);
+ final ImmutableBitSet rightBits =
+ ImmutableBitSet.range(left.getRowType().getFieldCount(),
+ join.getRowType().getFieldCount());
+ return !bits.intersects(rightBits);
+}
+
+@Override public void onMatch(RelOptRuleCall call) {
+ final Project project = call.rel(0);
+ final Join join = call.rel(1);
+ final RelNode left = call.rel(2);
+ final RelNode right = call.rel(3);
+
+ final JoinInfo joinInfo = join.analyzeCondition();
+ final RelOptCluster cluster = join.getCluster();
+ final RelMetadataQuery mq = cluster.getMetadataQuery();
+ final Boolean unique = mq.areColumnsUnique(right, joinInfo.rightSet());
+ if (unique != null && unique) {
+final RelBuilder builder = call.builder();
+switch (join.getJoinType()) {
+case INNER:
+ builder.push(left);
+ builder.push(right);
+ builder.join(JoinRelType.SEMI, join.getCondition());
+ break;
+case LEFT:
+ builder.push(left);
+ break;
+default:
+ throw new AssertionError(join.getJoinType());
+}
+builder.project(project.getProjects());
Review Comment:
@sdreynolds thanks for catching this, I noticed that you have opened
CALCITE-5391 to track this, let's go to there for further discussions.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] wnob commented on pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
wnob commented on PR #2973: URL: https://github.com/apache/calcite/pull/2973#issuecomment-1320707981 @tjbanghart > Wouldn't `DATETIME` map to Calcite's interpretation of a `TIMESTAMP` due to the lack of TZ info? Neither has tz info. I included a little table in the design doc that shows similarities between types in BQ and Calcite, and BQ `TIMESTAMP` maps pretty well to Calcite's `TIMESTAMP`. There isn't really a good existing type for `DATETIME` in Calcite. I think we'll have to implement one. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] wnob commented on a diff in pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
wnob commented on code in PR #2973:
URL: https://github.com/apache/calcite/pull/2973#discussion_r1026992374
##
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##
@@ -149,6 +159,40 @@ public class SqlFunctions {
private static final Pattern PATTERN_0_STAR_E = Pattern.compile("0*E");
+ // Date formatter for BigQuery's timestamp literals:
+ //
https://cloud.google.com/bigquery/docs/reference/standard-sql/lexical#timestamp_literals
+ private static final DateTimeFormatter BIG_QUERY_TIMESTAMP_LITERAL_FORMATTER
=
+ new DateTimeFormatterBuilder()
+ // Unlike ISO 8601, BQ only supports years between 1 - ,
+ // but can support single-digit month and day parts.
+ .appendValue(ChronoField.YEAR, 4)
+ .appendLiteral('-')
+ .appendValue(ChronoField.MONTH_OF_YEAR, 1, 2, SignStyle.NOT_NEGATIVE)
+ .appendLiteral('-')
+ .appendValue(ChronoField.DAY_OF_MONTH, 1, 2, SignStyle.NOT_NEGATIVE)
+ // Everything after the date is optional. Optional sections can be
nested.
+ .optionalStart()
+ // BQ accepts either a literal 'T' or a space to separate the date
from the time,
+ // so make the 'T' optional but pad with 1 space if it's omitted.
+ .padNext(1, ' ')
+ .optionalStart()
+ .appendLiteral('T')
+ .optionalEnd()
+ // Unlike ISO 8601, BQ can support single-digit hour, minute, and
second parts.
+ .appendValue(ChronoField.HOUR_OF_DAY, 1, 2, SignStyle.NOT_NEGATIVE)
+ .appendLiteral(':')
+ .appendValue(ChronoField.MINUTE_OF_HOUR, 1, 2,
SignStyle.NOT_NEGATIVE)
+ .appendLiteral(':')
+ .appendValue(ChronoField.SECOND_OF_MINUTE, 1, 2,
SignStyle.NOT_NEGATIVE)
+ // ISO 8601 supports up to nanosecond precision, but BQ only up to
microsecond.
+ .optionalStart()
+ .appendFraction(ChronoField.MICRO_OF_SECOND, 0, 6, true)
+ .optionalEnd()
+ .optionalStart()
+ .parseLenient()
+ .appendOffsetId()
+ .toFormatter(Locale.ROOT);
+
Review Comment:
Yes, it would appear that JDK8 had a different implementation for
`DateTimeFormatter` that cannot handle offsets (or at least not in the same way
as later versions). I'm inclined to switch to regex-based conversion, which
will require more logic but be more widely compatible. Thoughts?
##
site/_docs/reference.md:
##
@@ -2596,7 +2596,16 @@ semantics.
| p | CONVERT_TIMEZONE(tz1, tz2, datetime) | Converts the timezone
of *datetime* from *tz1* to *tz2*
| b | CURRENT_DATETIME([timezone]) | Returns the current
time as a TIMESTAMP from *timezone*
| m | DAYNAME(datetime) | Returns the name, in
the connection's locale, of the weekday in *datetime*; for example, it returns
'星期日' for both DATE '2020-02-10' and TIMESTAMP '2020-02-10 10:10:10'
-| b | DATE(string) | Equivalent to
`CAST(string AS DATE)`
+| b | DATE(integer, integer, integer)| Returns a date object
given year, month, and day
+| b | DATE(timestamp)| Extracts the UTC date
from a timestamp
+| b | DATE(timestamp, string)| Extracts the date from
a timestamp, in a given time zone
+| b | TIMESTAMP(string) | Equivalent to
`CAST(string AS TIMESTAMP)`
+| b | TIMESTAMP(string, string) | Equivalent to
`CAST(string AS TIMESTAMP)`, converted to a given time zone
+| b | TIMESTAMP(date)| Convert a UTC date to a
timestamp (at midnight)
+| b | TIMESTAMP(date, string)| Convert a date to a
timestamp (at midnight), in a given time zone
+| b | TIME(integer, integer, integer)| Returns a time object
given hour, minute, and second
+| b | TIME(timestamp)| Extracts the UTC time
from a timestamp
Review Comment:
Reworded for clarity.
##
babel/src/main/codegen/includes/parserImpls.ftl:
##
@@ -42,6 +42,46 @@ SqlNode DateFunctionCall() :
}
}
+SqlNode TimestampFunctionCall() :
+{
+final SqlFunctionCategory funcType =
SqlFunctionCategory.USER_DEFINED_FUNCTION;
Review Comment:
Ya this was just copied from the existing `DateFunctionCall`. I assumed this
vaguely meant library function since there is no obvious "library function"
value in the `SqlFunctionCategory` enum. Perhaps these should all be in the
`TIMEDATE` category. I suppose since this is the babel parser, there's no real
concept of a library function.
BTW, is my assessment of babel in my above comment with Oliver correct?
Should we be putting these productions in the core parser instead (or in
addition to here)?
##
babel/src/test/resources/sql/big-query.iq:
##
@@ -1051,29 +1051,98 @@ select
[GitHub] [calcite] sdreynolds opened a new pull request, #2975: [CALCITE-5391] Preserve Project field names
sdreynolds opened a new pull request, #2975: URL: https://github.com/apache/calcite/pull/2975 ## Why: When rewriting the query for a semi join, the `JoinOnUniqueToSemiJoinRule` pushes a new project via `RelBuilder`. This project is *almost* a good copy of the original `Project` -- it is just missing the field names. ## How: This change uses `RelDataType` of the `Project` to get the field names for the new `Project` pushed on the `Relbuilder` stack. This is similar to what is done in `SemiJoinRule#perform` -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] sdreynolds commented on a diff in pull request #2848: [CALCITE-5201] Improve SemiJoinRule to match Join's right input which is unique for Join keys
sdreynolds commented on code in PR #2848:
URL: https://github.com/apache/calcite/pull/2848#discussion_r1026959819
##
core/src/main/java/org/apache/calcite/rel/rules/SemiJoinRule.java:
##
@@ -232,6 +234,84 @@ default JoinToSemiJoinRuleConfig
withOperandFor(Class joinClass,
}
}
+ /**
+ * SemiJoinRule that matches a Project on top of a Join with a RelNode
+ * which is unique for Join's right keys.
+ *
+ * @see CoreRules#JOIN_ON_UNIQUE_TO_SEMI_JOIN */
+ public static class JoinOnUniqueToSemiJoinRule extends SemiJoinRule {
+
+/** Creates a JoinOnUniqueToSemiJoinRule. */
+protected JoinOnUniqueToSemiJoinRule(JoinOnUniqueToSemiJoinRuleConfig
config) {
+ super(config);
+}
+
+@Override public boolean matches(RelOptRuleCall call) {
+ final Project project = call.rel(0);
+ final Join join = call.rel(1);
+ final RelNode left = call.rel(2);
+
+ final ImmutableBitSet bits =
+ RelOptUtil.InputFinder.bits(project.getProjects(), null);
+ final ImmutableBitSet rightBits =
+ ImmutableBitSet.range(left.getRowType().getFieldCount(),
+ join.getRowType().getFieldCount());
+ return !bits.intersects(rightBits);
+}
+
+@Override public void onMatch(RelOptRuleCall call) {
+ final Project project = call.rel(0);
+ final Join join = call.rel(1);
+ final RelNode left = call.rel(2);
+ final RelNode right = call.rel(3);
+
+ final JoinInfo joinInfo = join.analyzeCondition();
+ final RelOptCluster cluster = join.getCluster();
+ final RelMetadataQuery mq = cluster.getMetadataQuery();
+ final Boolean unique = mq.areColumnsUnique(right, joinInfo.rightSet());
+ if (unique != null && unique) {
+final RelBuilder builder = call.builder();
+switch (join.getJoinType()) {
+case INNER:
+ builder.push(left);
+ builder.push(right);
+ builder.join(JoinRelType.SEMI, join.getCondition());
+ break;
+case LEFT:
+ builder.push(left);
+ break;
+default:
+ throw new AssertionError(join.getJoinType());
+}
+builder.project(project.getProjects());
Review Comment:
:doh: this doesn't carry through the field aliases like it does in the other
[rule](https://github.com/libenchao/calcite/blob/65e07f71c3e98115f114ee05986daa6baab28b96/core/src/main/java/org/apache/calcite/rel/rules/SemiJoinRule.java#L127)
```java
builder.project(project.getProjects(), project.getRowType().getFieldNames());
```
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] julianhyde commented on a diff in pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
julianhyde commented on code in PR #2973:
URL: https://github.com/apache/calcite/pull/2973#discussion_r1026775499
##
site/_docs/reference.md:
##
@@ -2596,7 +2596,16 @@ semantics.
| p | CONVERT_TIMEZONE(tz1, tz2, datetime) | Converts the timezone
of *datetime* from *tz1* to *tz2*
| b | CURRENT_DATETIME([timezone]) | Returns the current
time as a TIMESTAMP from *timezone*
| m | DAYNAME(datetime) | Returns the name, in
the connection's locale, of the weekday in *datetime*; for example, it returns
'星期日' for both DATE '2020-02-10' and TIMESTAMP '2020-02-10 10:10:10'
-| b | DATE(string) | Equivalent to
`CAST(string AS DATE)`
+| b | DATE(integer, integer, integer)| Returns a date object
given year, month, and day
+| b | DATE(timestamp)| Extracts the UTC date
from a timestamp
+| b | DATE(timestamp, string)| Extracts the date from
a timestamp, in a given time zone
+| b | TIMESTAMP(string) | Equivalent to
`CAST(string AS TIMESTAMP)`
+| b | TIMESTAMP(string, string) | Equivalent to
`CAST(string AS TIMESTAMP)`, converted to a given time zone
+| b | TIMESTAMP(date)| Convert a UTC date to a
timestamp (at midnight)
+| b | TIMESTAMP(date, string)| Convert a date to a
timestamp (at midnight), in a given time zone
+| b | TIME(integer, integer, integer)| Returns a time object
given hour, minute, and second
+| b | TIME(timestamp)| Extracts the UTC time
from a timestamp
Review Comment:
A Calcite TIMESTAMP value does not have a UTC time. Even though you're
implementing for BQ, you must use Calcite terminology here.
##
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##
@@ -149,6 +159,40 @@ public class SqlFunctions {
private static final Pattern PATTERN_0_STAR_E = Pattern.compile("0*E");
+ // Date formatter for BigQuery's timestamp literals:
+ //
https://cloud.google.com/bigquery/docs/reference/standard-sql/lexical#timestamp_literals
+ private static final DateTimeFormatter BIG_QUERY_TIMESTAMP_LITERAL_FORMATTER
=
+ new DateTimeFormatterBuilder()
+ // Unlike ISO 8601, BQ only supports years between 1 - ,
+ // but can support single-digit month and day parts.
+ .appendValue(ChronoField.YEAR, 4)
+ .appendLiteral('-')
+ .appendValue(ChronoField.MONTH_OF_YEAR, 1, 2, SignStyle.NOT_NEGATIVE)
+ .appendLiteral('-')
+ .appendValue(ChronoField.DAY_OF_MONTH, 1, 2, SignStyle.NOT_NEGATIVE)
+ // Everything after the date is optional. Optional sections can be
nested.
+ .optionalStart()
+ // BQ accepts either a literal 'T' or a space to separate the date
from the time,
+ // so make the 'T' optional but pad with 1 space if it's omitted.
+ .padNext(1, ' ')
+ .optionalStart()
+ .appendLiteral('T')
+ .optionalEnd()
+ // Unlike ISO 8601, BQ can support single-digit hour, minute, and
second parts.
+ .appendValue(ChronoField.HOUR_OF_DAY, 1, 2, SignStyle.NOT_NEGATIVE)
+ .appendLiteral(':')
+ .appendValue(ChronoField.MINUTE_OF_HOUR, 1, 2,
SignStyle.NOT_NEGATIVE)
+ .appendLiteral(':')
+ .appendValue(ChronoField.SECOND_OF_MINUTE, 1, 2,
SignStyle.NOT_NEGATIVE)
+ // ISO 8601 supports up to nanosecond precision, but BQ only up to
microsecond.
+ .optionalStart()
+ .appendFraction(ChronoField.MICRO_OF_SECOND, 0, 6, true)
+ .optionalEnd()
+ .optionalStart()
+ .parseLenient()
+ .appendOffsetId()
+ .toFormatter(Locale.ROOT);
+
Review Comment:
is this the code that Will said was non-JDK8 compliant?
##
site/_docs/reference.md:
##
@@ -2596,7 +2596,16 @@ semantics.
| p | CONVERT_TIMEZONE(tz1, tz2, datetime) | Converts the timezone
of *datetime* from *tz1* to *tz2*
| b | CURRENT_DATETIME([timezone]) | Returns the current
time as a TIMESTAMP from *timezone*
| m | DAYNAME(datetime) | Returns the name, in
the connection's locale, of the weekday in *datetime*; for example, it returns
'星期日' for both DATE '2020-02-10' and TIMESTAMP '2020-02-10 10:10:10'
-| b | DATE(string) | Equivalent to
`CAST(string AS DATE)`
+| b | DATE(integer, integer, integer)| Returns a date object
given year, month, and day
Review Comment:
this list should be alphabetically sorted
##
core/src/main/java/org/apache/calcite/sql/fun/SqlDateFunction.java:
##
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license
[GitHub] [calcite] tjbanghart commented on pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
tjbanghart commented on PR #2973: URL: https://github.com/apache/calcite/pull/2973#issuecomment-1319499323 Wouldn't `DATETIME` map to Calcite's interpretation of a `TIMESTAMP` due to the lack of TZ info? Also, should we move this into the core parser? I don't feel strongly either way (or if it matters all that much) but the BQ style literals and some functions live in core already. Otherwise, LGTM and a great example for adding other functions. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] jbalint merged pull request #2874: [CALCITE-5239] Site: JDBC Adapter's current limitations is incorrect
jbalint merged PR #2874: URL: https://github.com/apache/calcite/pull/2874 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] wnob commented on pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
wnob commented on PR #2973: URL: https://github.com/apache/calcite/pull/2973#issuecomment-1319428833 I've now implemented all the overloads that do not involve `DATETIME` objects. I'm not sure what is the best way to proceed with these because there doesn't seem to be a good fit in standard SQL to act as an alias. `TIMESTAMP_WITH_LOCAL_TIME_ZONE` is not a great fit; it has an implied time zone, whereas a `DATETIME` has no time zone at all. It's not necessary for MVP since we don't yet support `DATETIME` at all, so I'm inclined to punt on this for now. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] wnob commented on a diff in pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
wnob commented on code in PR #2973: URL: https://github.com/apache/calcite/pull/2973#discussion_r1025778875 ## babel/src/main/codegen/includes/parserImpls.ftl: ## @@ -42,6 +42,26 @@ SqlNode DateFunctionCall() : } } +SqlNode TimestampFunctionCall() : Review Comment: Here's my understanding: Babel is the "universal" parser ¹ that does not understand the semantics of individual dialects (hence the name). Dialect-specific semantic validation is a separate step that occurs after dialect-agnostic parsing. This is a [FreeMarker template file][1]. It does not contain any interpolation or tags, so running this through FreeMarker would simply remove the license comment at the top of the file. It's processed by [FMPP][2] in `config.fmpp` [here][3], which is in-turn referenced in the [JavaCC][4] grammar [here][5], as well as [here][6] via the `builtinFunctionCallMethods` list where the function name was added above in `config.fmpp`. Together, these 2 additions to the grammar indicate that there is a *production* called `TimestampFunctionCall` which consists of the [`` token][7] followed by a [`FunctionParameterList`][8] production, which consists of left-parenthesis, comma-separated arguments, right-parenthesis. The parser does nothing to validate how many arguments there are or what types they are. So yes, long story short, you're right that we didn't have to modify the `DateFunctionCall` production because it's just a function call either way, and I just added this new production with a different name token but which is otherwise identical to the `DATE` one. ¹ I've heard Julian make reference to other parsers being potentially usable with Calcite but I don't know of any examples. [1]: https://freemarker.apache.org/docs/dgui_template_overallstructure.html [2]: https://fmpp.sourceforge.net/index.html [3]: https://github.com/apache/calcite/blob/a505b25eacc473c6ec0ef8abd40c1ccae86297b6/babel/src/main/codegen/config.fmpp#L566 [4]: https://javacc.github.io/javacc/ [5]: https://github.com/apache/calcite/blob/a0e119ea42def418957f214f539469f1aba76c18/core/src/main/codegen/templates/Parser.jj#L1163 [6]: https://github.com/apache/calcite/blob/a0e119ea42def418957f214f539469f1aba76c18/core/src/main/codegen/templates/Parser.jj#L5992 [7]: https://github.com/apache/calcite/blob/a0e119ea42def418957f214f539469f1aba76c18/core/src/main/codegen/templates/Parser.jj#L7922 [8]: https://github.com/apache/calcite/blob/a0e119ea42def418957f214f539469f1aba76c18/core/src/main/codegen/templates/Parser.jj#L948 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite-avatica] snuyanzin merged pull request #196: [CALCITE-5379] Upgrade protobuf to 3.21.9
snuyanzin merged PR #196: URL: https://github.com/apache/calcite-avatica/pull/196 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] olivrlee commented on a diff in pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
olivrlee commented on code in PR #2973: URL: https://github.com/apache/calcite/pull/2973#discussion_r1024660113 ## babel/src/main/codegen/includes/parserImpls.ftl: ## @@ -42,6 +42,26 @@ SqlNode DateFunctionCall() : } } +SqlNode TimestampFunctionCall() : Review Comment: Can you explain a bit (to me, not needing comments on the file )on the whole purpose of the `/babel` directory? What I'm understanding here is that its used to be able to parse new SQL statements coming in, the `date` function existed already above this block so it doesn't need to be added -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] olivrlee commented on a diff in pull request #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
olivrlee commented on code in PR #2973: URL: https://github.com/apache/calcite/pull/2973#discussion_r1024660113 ## babel/src/main/codegen/includes/parserImpls.ftl: ## @@ -42,6 +42,26 @@ SqlNode DateFunctionCall() : } } +SqlNode TimestampFunctionCall() : Review Comment: Can you explain a bit on the whole purpose of the `/babel` directory? What I'm understanding here is that its used to be able to parse new SQL statements coming in, the `date` function existed already above this block so it doesn't need to be added -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] wnob opened a new pull request, #2973: [CALCITE-5180] Implement some of the overloads for BigQuery's DATE and TIMESTAMP
wnob opened a new pull request, #2973: URL: https://github.com/apache/calcite/pull/2973 There is an existing implementation for a BQ `DATE` function but it wrongly thinks this is simply a typecast. See documentation [here][1]. This PR implements 2 of the 4 possible overloads for `DATE` (considering the optional time zone in the second form) as well as 1 of the possible overloads for `TIMESTAMP`, and was able to get a quidem test to pass utilizing both functions. I intend to implement the rest of the overloads in a follow-up commit but was hoping for some feedback now and thought this was a decent-sized commit to review at once. [1]: https://cloud.google.com/bigquery/docs/reference/standard-sql/date_functions#date -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] k-jamroz commented on pull request #2878: [CALCITE-5241] Implement CHAR function
k-jamroz commented on PR #2878: URL: https://github.com/apache/calcite/pull/2878#issuecomment-1317373320 Interesting changes in 1.32.0: * https://issues.apache.org/jira/browse/CALCITE-5241 - `CHAR` is now a reserved function name. I am not sure if this may impact parsing. Maybe we want to implement this function? * https://issues.apache.org/jira/browse/CALCITE-5232 (Upgrade protobuf-java from 3.17.1 to 3.21.5) - we currently have 3.21.9, so should be fine * https://issues.apache.org/jira/browse/CALCITE-5178 - we currently do not support scalar queries, should not impact us -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] julianhyde closed pull request #2965: [CALCITE-5105] Add MEASURE type and AGGREGATE aggregate function
julianhyde closed pull request #2965: [CALCITE-5105] Add MEASURE type and AGGREGATE aggregate function URL: https://github.com/apache/calcite/pull/2965 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] freastro opened a new pull request, #2972: [CALCITE-2989] Use ISO calendar system when converting from java.sql.Date/Time/Timestamp to a unix timestamp
freastro opened a new pull request, #2972: URL: https://github.com/apache/calcite/pull/2972 This changes the unix timestamp to Date/Time/Timestamp conversion in `SqlFunctions` to convert between the ISO calendar system and the standard Gregorian calendar. This ensures that unix timestamps created by `SqlFunctions` will have the same value as unix timestamps created by `DateTimeUtils`. Some of the tests may fail without this corresponding change to Avatica: https://github.com/apache/calcite-avatica/pull/197 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite-avatica] freastro opened a new pull request, #197: [CALCITE-2989] Use ISO calendar system when accessing Date/Time/Timestamp from a number
freastro opened a new pull request, #197: URL: https://github.com/apache/calcite-avatica/pull/197 This changes the unix timestamp to Date/Time/Timestamp conversion in `AbstractCursor` to convert between the ISO calendar system and the standard Gregorian calendar. This ensures that unix timestamps produced by `DateTimeUtils` are correctly converted to `java.sql.Date/Time/Timestamp` objects. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] libenchao closed pull request #2929: [CALCITE-5310] JSON_OBJECT in scalar sub-query throws AssertionError
libenchao closed pull request #2929: [CALCITE-5310] JSON_OBJECT in scalar sub-query throws AssertionError URL: https://github.com/apache/calcite/pull/2929 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #2929: [CALCITE-5310] JSON_OBJECT in scalar sub-query throws AssertionError
libenchao commented on code in PR #2929:
URL: https://github.com/apache/calcite/pull/2929#discussion_r1022368836
##
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##
@@ -1790,10 +1790,11 @@ private RexNode createCaseExpression(
@Override public RexNode visitLiteral(RexLiteral literal) {
// Use nullIndicator to decide whether to project null.
- // Do nothing if the literal is null.
+ // Do nothing if the literal is null or symbol.
if (!RexUtil.isNull(literal)
Review Comment:
@dssysolyatin Thanks for your review.
It's good to have such questions and discussions.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] Pritam-Gote opened a new pull request, #2971: RTP 180 Support For MOD Function on Date Field
Pritam-Gote opened a new pull request, #2971: URL: https://github.com/apache/calcite/pull/2971 RTP 180 support for mod function on date field -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2929: [CALCITE-5310] JSON_OBJECT in scalar sub-query throws AssertionError
dssysolyatin commented on code in PR #2929:
URL: https://github.com/apache/calcite/pull/2929#discussion_r1022331269
##
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##
@@ -1790,10 +1790,11 @@ private RexNode createCaseExpression(
@Override public RexNode visitLiteral(RexLiteral literal) {
// Use nullIndicator to decide whether to project null.
- // Do nothing if the literal is null.
+ // Do nothing if the literal is null or symbol.
if (!RexUtil.isNull(literal)
Review Comment:
Ok, I was just trying to understand if we can reduce number of
caseExpression. Anyway it is not directly related to this ticket.
LGTM
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] libenchao closed pull request #2970: [CALCITE-5383] Update CONCAT_FUNCTION SqlLibary to include BIG_QUERY
libenchao closed pull request #2970: [CALCITE-5383] Update CONCAT_FUNCTION SqlLibary to include BIG_QUERY URL: https://github.com/apache/calcite/pull/2970 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #2929: [CALCITE-5310] JSON_OBJECT in scalar sub-query throws AssertionError
libenchao commented on code in PR #2929:
URL: https://github.com/apache/calcite/pull/2929#discussion_r1022252807
##
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##
@@ -1790,10 +1790,11 @@ private RexNode createCaseExpression(
@Override public RexNode visitLiteral(RexLiteral literal) {
// Use nullIndicator to decide whether to project null.
- // Do nothing if the literal is null.
+ // Do nothing if the literal is null or symbol.
if (!RexUtil.isNull(literal)
Review Comment:
The whole story lies in `RelDecorrelator#RemoveCorrelationRexShuttle`. When
`projectPulledAboveLeftCorrelator && (nullIndicator != null)` is `true`,
currently it will add a case for all expressions.
Maybe your feeling is right, I haven't thought it thorough.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2929: [CALCITE-5310] JSON_OBJECT in scalar sub-query throws AssertionError
dssysolyatin commented on code in PR #2929:
URL: https://github.com/apache/calcite/pull/2929#discussion_r1021396407
##
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##
@@ -1790,10 +1790,11 @@ private RexNode createCaseExpression(
@Override public RexNode visitLiteral(RexLiteral literal) {
// Use nullIndicator to decide whether to project null.
- // Do nothing if the literal is null.
+ // Do nothing if the literal is null or symbol.
if (!RexUtil.isNull(literal)
Review Comment:
Ok, I got why calcite wraps literals if literal is in SELECT list like
`SELEC (SELECT 1 )`. But I still don't understand why `CARDINALITY`
arguments should be wrapped in case expression ?
```
CARDINALITY(ARRAY[false, true, a.attidentity = 'a'])
```
In my understanding only CARDINALITY should be wrapped to caseExpression
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] vijayjogi-dm opened a new pull request, #2969: Vijay.jogi/ravcom 40 wf vulnerabilities issues
vijayjogi-dm opened a new pull request, #2969: URL: https://github.com/apache/calcite/pull/2969 Carry over changes from https://github.com/apache/calcite/commit/ba80b9156afc0db26b194d97a031fcc0dc7f4c03# commit to fix CVE-2022-39135 Vulnerability issue -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020909779
##
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##
@@ -3027,6 +3028,43 @@ private void checkSarg(String message, Sarg sarg,
is(false));
}
+ private void helpTestFloorCeil(String timestampString, String
expectedFloorString,
+ String expectedCeilString, TimeUnitRange timeUnitRange) {
+final RexLiteral timestampLiteral = rexBuilder.makeTimestampLiteral(
+new TimestampString(timestampString), 3);
+final RexLiteral flagLiteral = rexBuilder.makeFlag(timeUnitRange);
+
+RexNode flooredLiteral = rexBuilder.makeCall(SqlStdOperatorTable.FLOOR,
+ImmutableList.of(timestampLiteral, flagLiteral));
+long expectedFloorValue = new
TimestampString(expectedFloorString).getMillisSinceEpoch();
+assertThat(eval(flooredLiteral), is(expectedFloorValue));
+
+RexNode ceiledLiteral = rexBuilder.makeCall(SqlStdOperatorTable.CEIL,
+ImmutableList.of(timestampLiteral, flagLiteral));
+long expectedCeiledFloorValue = new
TimestampString(expectedCeilString).getMillisSinceEpoch();
+assertThat(eval(ceiledLiteral), is(expectedCeiledFloorValue));
+ }
+
+ @Test void testInterpreterFloorCeil() {
+String timestampString = "2011-07-20 12:34:18.078";
+helpTestFloorCeil(timestampString, "2011-07-20 12:34:18", "2011-07-20
12:34:19",
+TimeUnitRange.SECOND);
+helpTestFloorCeil(timestampString, "2011-07-20 12:34:00", "2011-07-20
12:35:00",
+TimeUnitRange.MINUTE);
+helpTestFloorCeil(timestampString, "2011-07-20 12:00:00", "2011-07-20
13:00:00",
+TimeUnitRange.HOUR);
+helpTestFloorCeil(timestampString, "2011-07-20 00:00:00", "2011-07-21
00:00:00",
+TimeUnitRange.DAY);
+helpTestFloorCeil("2011-07-20 12:34:59.078", "2011-07-17 00:00:00",
"2011-07-24 00:00:00",
Review Comment:
The start of week here is on Sunday, which is locale dependant, is there a
way to make this locale dependant? If not, why would it be OK as is?
##
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##
@@ -3027,6 +3028,43 @@ private void checkSarg(String message, Sarg sarg,
is(false));
}
+ private void helpTestFloorCeil(String timestampString, String
expectedFloorString,
+ String expectedCeilString, TimeUnitRange timeUnitRange) {
+final RexLiteral timestampLiteral = rexBuilder.makeTimestampLiteral(
+new TimestampString(timestampString), 3);
+final RexLiteral flagLiteral = rexBuilder.makeFlag(timeUnitRange);
+
+RexNode flooredLiteral = rexBuilder.makeCall(SqlStdOperatorTable.FLOOR,
+ImmutableList.of(timestampLiteral, flagLiteral));
+long expectedFloorValue = new
TimestampString(expectedFloorString).getMillisSinceEpoch();
+assertThat(eval(flooredLiteral), is(expectedFloorValue));
+
+RexNode ceiledLiteral = rexBuilder.makeCall(SqlStdOperatorTable.CEIL,
Review Comment:
Please be consistent with the `final` modifier, since you have added it on
some other vars, if you omit it here, the reader thinks it's going to mutate,
while this is not the case
##
core/src/test/java/org/apache/calcite/test/TestMetadataHandlers.java:
##
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.test;
+
+import org.apache.calcite.avatica.util.DateTimeUtils;
+import org.apache.calcite.plan.RelOptPredicateList;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.core.TableScan;
+import org.apache.calcite.rel.metadata.BuiltInMetadata;
+import org.apache.calcite.rel.metadata.ReflectiveRelMetadataProvider;
+import org.apache.calcite.rel.metadata.RelMdRowCount;
+import org.apache.calcite.rel.metadata.RelMdSelectivity;
+import org.apache.calcite.rel.metadata.RelMdUtil;
+import org.apache.calcite.rel.metadata.RelMetadataProvider;
+import org.apache.calcite.rel.metadata.RelMetadataQuery;
+import org.apache.calcite.rex.RexCall;
+import org.apache.calcite.rex.RexExecutor;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexSimplify;
+import
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020908879
##
core/src/test/java/org/apache/calcite/rex/RexProgramTest.java:
##
@@ -3027,6 +3028,43 @@ private void checkSarg(String message, Sarg sarg,
is(false));
}
+ private void helpTestFloorCeil(String timestampString, String
expectedFloorString,
+ String expectedCeilString, TimeUnitRange timeUnitRange) {
+final RexLiteral timestampLiteral = rexBuilder.makeTimestampLiteral(
+new TimestampString(timestampString), 3);
Review Comment:
Is precision `3` always appropriate?
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876: URL: https://github.com/apache/calcite/pull/2876#discussion_r1020908635 ## core/src/test/java/org/apache/calcite/rex/RexProgramTest.java: ## @@ -3027,6 +3028,43 @@ private void checkSarg(String message, Sarg sarg, is(false)); } + private void helpTestFloorCeil(String timestampString, String expectedFloorString, Review Comment: `assertFloorCeil` or `checkFloorCeil`? -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876: URL: https://github.com/apache/calcite/pull/2876#discussion_r1020908635 ## core/src/test/java/org/apache/calcite/rex/RexProgramTest.java: ## @@ -3027,6 +3028,43 @@ private void checkSarg(String message, Sarg sarg, is(false)); } + private void helpTestFloorCeil(String timestampString, String expectedFloorString, Review Comment: `assertFloorCeil`? -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020908293
##
core/src/main/java/org/apache/calcite/rex/RexInterpreter.java:
##
@@ -342,22 +346,15 @@ private static Comparable ceil(RexCall call,
List values) {
default:
break;
}
-final TimeUnitRange subUnit = subUnit(unit);
-for (long v2 = v;;) {
- final int e = DateTimeUtils.unixTimestampExtract(subUnit, v2);
- if (e == 0) {
-return v2;
- }
- v2 -= unit.startUnit.multiplier.longValue();
-}
+return ceilSubDay(call.getKind(), v, unit);
}
- private static TimeUnitRange subUnit(TimeUnitRange unit) {
-switch (unit) {
-case QUARTER:
- return TimeUnitRange.MONTH;
+ private static long ceilSubDay(SqlKind kind, Long v, TimeUnitRange unit) {
+switch (kind) {
+case FLOOR:
+ return SqlFunctions.floor(v, unit.startUnit.multiplier.longValue());
default:
- return TimeUnitRange.DAY;
+ return SqlFunctions.ceil(v, unit.startUnit.multiplier.longValue());
Review Comment:
If the `default` case is just for `ceil`, I'd rather cover it explicitly
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020901059
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -1252,6 +1285,193 @@ protected RexNode replaceWithOriginalReferences(final
RexBuilder rexBuilder,
}
}
+ /**
+ * Used to generate a view predicate that is added to a materialized view
that aggregates
+ * over a FLOOR(datetime) when the query has a range predicate on the same
column.
+ */
+ static class ImplicitViewPredicateShuttle extends RexShuttle {
+
+private final RexBuilder rexBuilder;
+private final RexCall floorCall;
+private final RexInputRef rexInputRef;
+private final boolean generateViewFilter;
+private long lowerBound;
+private long upperBound;
+
+ImplicitViewPredicateShuttle(RexBuilder rexBuilder, RexCall floorCall,
+RexInputRef rexInputRef,
+boolean generateViewFilter) {
+ this.floorCall = floorCall;
+ this.rexBuilder = rexBuilder;
+ this.rexInputRef = rexInputRef;
+ this.generateViewFilter = generateViewFilter;
+}
+
+private RexNode transformCall(RexCall call, boolean isLowerBound) {
+ SqlOperator transformedCallOperator = isLowerBound
+ ? SqlStdOperatorTable.GREATER_THAN_OR_EQUAL :
SqlStdOperatorTable.LESS_THAN;
+ // matches functions of the form x > 5 or 5 > x
+ RexNode literalOperand = call.operands.get(0);
+ RexNode tableInputRefOperand = call.operands.get(1);
+ final int floorIndex = ((RexInputRef)
floorCall.getOperands().get(0)).getIndex();
+ boolean reverseOperands = false;
+ if ((literalOperand.getKind() == SqlKind.LITERAL
+ && tableInputRefOperand.getKind() == SqlKind.TABLE_INPUT_REF)
+ || (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+ && tableInputRefOperand.getKind() == SqlKind.LITERAL)) {
+if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+&& tableInputRefOperand.getKind() == SqlKind.LITERAL) {
+ literalOperand = call.operands.get(1);
+ tableInputRefOperand = call.operands.get(0);
+ reverseOperands = true;
+}
+int predicateIndex = ((RexTableInputRef)
tableInputRefOperand).getIndex();
+if (floorIndex == predicateIndex) {
+ // if the query predicate contains a range over the column that is
floored in the
+ // materialized view we can generate a filter on the view
+ boolean shiftTruncatedVal = call.getOperator() !=
transformedCallOperator;
+ long truncatedVal = getModifiedVal(shiftTruncatedVal, isLowerBound,
literalOperand);
+ RexNode truncatedLiteral =
+
rexBuilder.makeTimestampLiteral(TimestampString.fromMillisSinceEpoch(truncatedVal),
+ 0);
+ if (isLowerBound) {
+lowerBound = truncatedVal;
+ } else {
+upperBound = truncatedVal;
+ }
+ if (generateViewFilter) {
+tableInputRefOperand = rexInputRef;
+ }
+ RexNode modifiedCall = rexBuilder.makeCall(call.getType(),
transformedCallOperator,
+ reverseOperands ? ImmutableList.of(tableInputRefOperand,
truncatedLiteral)
+ : ImmutableList.of(truncatedLiteral, tableInputRefOperand));
+ return modifiedCall;
+} else {
+ // ignore predicates on different columns
+ return rexBuilder.makeLiteral(true);
+}
+ }
+ return super.visitCall(call);
+}
+
+private long getModifiedVal(boolean shiftModifiedVal, boolean isLowerBound,
+RexNode literalOperand) {
+ SqlOperator floorOperator = isLowerBound ? SqlStdOperatorTable.CEIL
+ : SqlStdOperatorTable.FLOOR;
+ RexNode truncatedLiteral = rexBuilder.makeCall(floorCall.getType(),
+ floorOperator, ImmutableList.of(literalOperand,
floorCall.getOperands().get(1)));
+ Comparable v0 = RexInterpreter.evaluate(truncatedLiteral,
Collections.emptyMap());
+ if (v0 == null) {
+throw new AssertionError("interpreter returned null for " + v0);
+ }
+ Comparable v1 = RexInterpreter.evaluate(literalOperand,
Collections.emptyMap());
+ if (v1 == null) {
+throw new AssertionError("interpreter returned null for " + v1);
+ }
Review Comment:
`v0` and `v1` are always null when you build the assertion message, replace
`v0`/`v1` with `truncatedLiteral` and `literalOperand` (i.e., what you were
evaluating when `null` was returned)
Moreover, sentences should start with a capital letter
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020905457
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -1252,6 +1285,193 @@ protected RexNode replaceWithOriginalReferences(final
RexBuilder rexBuilder,
}
}
+ /**
+ * Used to generate a view predicate that is added to a materialized view
that aggregates
+ * over a FLOOR(datetime) when the query has a range predicate on the same
column.
+ */
+ static class ImplicitViewPredicateShuttle extends RexShuttle {
+
+private final RexBuilder rexBuilder;
+private final RexCall floorCall;
+private final RexInputRef rexInputRef;
+private final boolean generateViewFilter;
+private long lowerBound;
+private long upperBound;
+
+ImplicitViewPredicateShuttle(RexBuilder rexBuilder, RexCall floorCall,
+RexInputRef rexInputRef,
+boolean generateViewFilter) {
+ this.floorCall = floorCall;
+ this.rexBuilder = rexBuilder;
+ this.rexInputRef = rexInputRef;
+ this.generateViewFilter = generateViewFilter;
+}
+
+private RexNode transformCall(RexCall call, boolean isLowerBound) {
+ SqlOperator transformedCallOperator = isLowerBound
+ ? SqlStdOperatorTable.GREATER_THAN_OR_EQUAL :
SqlStdOperatorTable.LESS_THAN;
+ // matches functions of the form x > 5 or 5 > x
+ RexNode literalOperand = call.operands.get(0);
+ RexNode tableInputRefOperand = call.operands.get(1);
+ final int floorIndex = ((RexInputRef)
floorCall.getOperands().get(0)).getIndex();
+ boolean reverseOperands = false;
+ if ((literalOperand.getKind() == SqlKind.LITERAL
+ && tableInputRefOperand.getKind() == SqlKind.TABLE_INPUT_REF)
+ || (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+ && tableInputRefOperand.getKind() == SqlKind.LITERAL)) {
+if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+&& tableInputRefOperand.getKind() == SqlKind.LITERAL) {
+ literalOperand = call.operands.get(1);
+ tableInputRefOperand = call.operands.get(0);
+ reverseOperands = true;
+}
+int predicateIndex = ((RexTableInputRef)
tableInputRefOperand).getIndex();
+if (floorIndex == predicateIndex) {
+ // if the query predicate contains a range over the column that is
floored in the
+ // materialized view we can generate a filter on the view
+ boolean shiftTruncatedVal = call.getOperator() !=
transformedCallOperator;
+ long truncatedVal = getModifiedVal(shiftTruncatedVal, isLowerBound,
literalOperand);
+ RexNode truncatedLiteral =
+
rexBuilder.makeTimestampLiteral(TimestampString.fromMillisSinceEpoch(truncatedVal),
+ 0);
+ if (isLowerBound) {
+lowerBound = truncatedVal;
+ } else {
+upperBound = truncatedVal;
+ }
+ if (generateViewFilter) {
+tableInputRefOperand = rexInputRef;
+ }
+ RexNode modifiedCall = rexBuilder.makeCall(call.getType(),
transformedCallOperator,
+ reverseOperands ? ImmutableList.of(tableInputRefOperand,
truncatedLiteral)
+ : ImmutableList.of(truncatedLiteral, tableInputRefOperand));
+ return modifiedCall;
+} else {
+ // ignore predicates on different columns
+ return rexBuilder.makeLiteral(true);
+}
+ }
+ return super.visitCall(call);
+}
+
+private long getModifiedVal(boolean shiftModifiedVal, boolean isLowerBound,
+RexNode literalOperand) {
+ SqlOperator floorOperator = isLowerBound ? SqlStdOperatorTable.CEIL
+ : SqlStdOperatorTable.FLOOR;
+ RexNode truncatedLiteral = rexBuilder.makeCall(floorCall.getType(),
+ floorOperator, ImmutableList.of(literalOperand,
floorCall.getOperands().get(1)));
+ Comparable v0 = RexInterpreter.evaluate(truncatedLiteral,
Collections.emptyMap());
+ if (v0 == null) {
+throw new AssertionError("interpreter returned null for " + v0);
+ }
+ Comparable v1 = RexInterpreter.evaluate(literalOperand,
Collections.emptyMap());
+ if (v1 == null) {
+throw new AssertionError("interpreter returned null for " + v1);
+ }
+ long modifiedVal = (long) v0;
+ final long originalVal = (long) v1;
+ // Since the view contains a FLOOR() if the query does a > or <= and the
operand is already
+ // a FLOORED value we have to shift the value to the next higher or
lower floored value
+ // If the query contains a >= or < we don't need to shift the modified
value
+ if (shiftModifiedVal && modifiedVal == originalVal) {
+final RexLiteral literal = (RexLiteral) floorCall.getOperands().get(1);
+final TimeUnitRange unit =
castNonNull(literal.getValueAs(TimeUnitRange.class));
+if
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020904821
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -1252,6 +1285,193 @@ protected RexNode replaceWithOriginalReferences(final
RexBuilder rexBuilder,
}
}
+ /**
+ * Used to generate a view predicate that is added to a materialized view
that aggregates
+ * over a FLOOR(datetime) when the query has a range predicate on the same
column.
+ */
+ static class ImplicitViewPredicateShuttle extends RexShuttle {
+
+private final RexBuilder rexBuilder;
+private final RexCall floorCall;
+private final RexInputRef rexInputRef;
+private final boolean generateViewFilter;
+private long lowerBound;
+private long upperBound;
+
+ImplicitViewPredicateShuttle(RexBuilder rexBuilder, RexCall floorCall,
+RexInputRef rexInputRef,
+boolean generateViewFilter) {
+ this.floorCall = floorCall;
+ this.rexBuilder = rexBuilder;
+ this.rexInputRef = rexInputRef;
+ this.generateViewFilter = generateViewFilter;
+}
+
+private RexNode transformCall(RexCall call, boolean isLowerBound) {
Review Comment:
I rename `transformCall` into `adjustComparisonBoundary`,
`transformComparisonBoundary` or something closer to what this method is
actually doing
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020902630
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -1252,6 +1285,193 @@ protected RexNode replaceWithOriginalReferences(final
RexBuilder rexBuilder,
}
}
+ /**
+ * Used to generate a view predicate that is added to a materialized view
that aggregates
+ * over a FLOOR(datetime) when the query has a range predicate on the same
column.
+ */
+ static class ImplicitViewPredicateShuttle extends RexShuttle {
+
+private final RexBuilder rexBuilder;
+private final RexCall floorCall;
+private final RexInputRef rexInputRef;
+private final boolean generateViewFilter;
+private long lowerBound;
+private long upperBound;
+
+ImplicitViewPredicateShuttle(RexBuilder rexBuilder, RexCall floorCall,
+RexInputRef rexInputRef,
+boolean generateViewFilter) {
+ this.floorCall = floorCall;
+ this.rexBuilder = rexBuilder;
+ this.rexInputRef = rexInputRef;
+ this.generateViewFilter = generateViewFilter;
+}
+
+private RexNode transformCall(RexCall call, boolean isLowerBound) {
+ SqlOperator transformedCallOperator = isLowerBound
+ ? SqlStdOperatorTable.GREATER_THAN_OR_EQUAL :
SqlStdOperatorTable.LESS_THAN;
+ // matches functions of the form x > 5 or 5 > x
+ RexNode literalOperand = call.operands.get(0);
+ RexNode tableInputRefOperand = call.operands.get(1);
+ final int floorIndex = ((RexInputRef)
floorCall.getOperands().get(0)).getIndex();
+ boolean reverseOperands = false;
+ if ((literalOperand.getKind() == SqlKind.LITERAL
+ && tableInputRefOperand.getKind() == SqlKind.TABLE_INPUT_REF)
+ || (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+ && tableInputRefOperand.getKind() == SqlKind.LITERAL)) {
+if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+&& tableInputRefOperand.getKind() == SqlKind.LITERAL) {
+ literalOperand = call.operands.get(1);
+ tableInputRefOperand = call.operands.get(0);
+ reverseOperands = true;
+}
+int predicateIndex = ((RexTableInputRef)
tableInputRefOperand).getIndex();
+if (floorIndex == predicateIndex) {
+ // if the query predicate contains a range over the column that is
floored in the
+ // materialized view we can generate a filter on the view
+ boolean shiftTruncatedVal = call.getOperator() !=
transformedCallOperator;
+ long truncatedVal = getModifiedVal(shiftTruncatedVal, isLowerBound,
literalOperand);
+ RexNode truncatedLiteral =
+
rexBuilder.makeTimestampLiteral(TimestampString.fromMillisSinceEpoch(truncatedVal),
+ 0);
+ if (isLowerBound) {
+lowerBound = truncatedVal;
+ } else {
+upperBound = truncatedVal;
+ }
+ if (generateViewFilter) {
+tableInputRefOperand = rexInputRef;
+ }
+ RexNode modifiedCall = rexBuilder.makeCall(call.getType(),
transformedCallOperator,
+ reverseOperands ? ImmutableList.of(tableInputRefOperand,
truncatedLiteral)
+ : ImmutableList.of(truncatedLiteral, tableInputRefOperand));
+ return modifiedCall;
+} else {
+ // ignore predicates on different columns
+ return rexBuilder.makeLiteral(true);
+}
+ }
+ return super.visitCall(call);
+}
+
+private long getModifiedVal(boolean shiftModifiedVal, boolean isLowerBound,
+RexNode literalOperand) {
+ SqlOperator floorOperator = isLowerBound ? SqlStdOperatorTable.CEIL
+ : SqlStdOperatorTable.FLOOR;
+ RexNode truncatedLiteral = rexBuilder.makeCall(floorCall.getType(),
+ floorOperator, ImmutableList.of(literalOperand,
floorCall.getOperands().get(1)));
+ Comparable v0 = RexInterpreter.evaluate(truncatedLiteral,
Collections.emptyMap());
+ if (v0 == null) {
+throw new AssertionError("interpreter returned null for " + v0);
+ }
+ Comparable v1 = RexInterpreter.evaluate(literalOperand,
Collections.emptyMap());
+ if (v1 == null) {
+throw new AssertionError("interpreter returned null for " + v1);
+ }
+ long modifiedVal = (long) v0;
+ final long originalVal = (long) v1;
+ // Since the view contains a FLOOR() if the query does a > or <= and the
operand is already
+ // a FLOORED value we have to shift the value to the next higher or
lower floored value
+ // If the query contains a >= or < we don't need to shift the modified
value
+ if (shiftModifiedVal && modifiedVal == originalVal) {
+final RexLiteral literal = (RexLiteral) floorCall.getOperands().get(1);
+final TimeUnitRange unit =
castNonNull(literal.getValueAs(TimeUnitRange.class));
+if
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020902356
##
core/src/test/java/org/apache/calcite/test/MaterializedViewRelOptRulesTest.java:
##
@@ -338,6 +341,104 @@ protected final MaterializedViewFixture sql(String
materialize,
.ok();
}
+ @Test void testAggregateMaterializationAggregateFuncsRange1() {
Review Comment:
Please find better names for tests, if you have multiple tests, they are
covering something specific each, so why don't put that in the test name? This
is true for all tests1...n in the PR
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020901685
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -1252,6 +1285,193 @@ protected RexNode replaceWithOriginalReferences(final
RexBuilder rexBuilder,
}
}
+ /**
+ * Used to generate a view predicate that is added to a materialized view
that aggregates
+ * over a FLOOR(datetime) when the query has a range predicate on the same
column.
+ */
+ static class ImplicitViewPredicateShuttle extends RexShuttle {
+
+private final RexBuilder rexBuilder;
+private final RexCall floorCall;
+private final RexInputRef rexInputRef;
+private final boolean generateViewFilter;
+private long lowerBound;
+private long upperBound;
+
+ImplicitViewPredicateShuttle(RexBuilder rexBuilder, RexCall floorCall,
+RexInputRef rexInputRef,
+boolean generateViewFilter) {
+ this.floorCall = floorCall;
+ this.rexBuilder = rexBuilder;
+ this.rexInputRef = rexInputRef;
+ this.generateViewFilter = generateViewFilter;
+}
+
+private RexNode transformCall(RexCall call, boolean isLowerBound) {
+ SqlOperator transformedCallOperator = isLowerBound
+ ? SqlStdOperatorTable.GREATER_THAN_OR_EQUAL :
SqlStdOperatorTable.LESS_THAN;
+ // matches functions of the form x > 5 or 5 > x
+ RexNode literalOperand = call.operands.get(0);
+ RexNode tableInputRefOperand = call.operands.get(1);
+ final int floorIndex = ((RexInputRef)
floorCall.getOperands().get(0)).getIndex();
+ boolean reverseOperands = false;
+ if ((literalOperand.getKind() == SqlKind.LITERAL
+ && tableInputRefOperand.getKind() == SqlKind.TABLE_INPUT_REF)
+ || (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+ && tableInputRefOperand.getKind() == SqlKind.LITERAL)) {
+if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+&& tableInputRefOperand.getKind() == SqlKind.LITERAL) {
+ literalOperand = call.operands.get(1);
+ tableInputRefOperand = call.operands.get(0);
+ reverseOperands = true;
+}
+int predicateIndex = ((RexTableInputRef)
tableInputRefOperand).getIndex();
+if (floorIndex == predicateIndex) {
+ // if the query predicate contains a range over the column that is
floored in the
+ // materialized view we can generate a filter on the view
+ boolean shiftTruncatedVal = call.getOperator() !=
transformedCallOperator;
+ long truncatedVal = getModifiedVal(shiftTruncatedVal, isLowerBound,
literalOperand);
+ RexNode truncatedLiteral =
+
rexBuilder.makeTimestampLiteral(TimestampString.fromMillisSinceEpoch(truncatedVal),
+ 0);
+ if (isLowerBound) {
+lowerBound = truncatedVal;
+ } else {
+upperBound = truncatedVal;
+ }
+ if (generateViewFilter) {
+tableInputRefOperand = rexInputRef;
+ }
+ RexNode modifiedCall = rexBuilder.makeCall(call.getType(),
transformedCallOperator,
+ reverseOperands ? ImmutableList.of(tableInputRefOperand,
truncatedLiteral)
+ : ImmutableList.of(truncatedLiteral, tableInputRefOperand));
+ return modifiedCall;
+} else {
+ // ignore predicates on different columns
+ return rexBuilder.makeLiteral(true);
+}
+ }
+ return super.visitCall(call);
+}
+
+private long getModifiedVal(boolean shiftModifiedVal, boolean isLowerBound,
+RexNode literalOperand) {
+ SqlOperator floorOperator = isLowerBound ? SqlStdOperatorTable.CEIL
+ : SqlStdOperatorTable.FLOOR;
+ RexNode truncatedLiteral = rexBuilder.makeCall(floorCall.getType(),
+ floorOperator, ImmutableList.of(literalOperand,
floorCall.getOperands().get(1)));
+ Comparable v0 = RexInterpreter.evaluate(truncatedLiteral,
Collections.emptyMap());
+ if (v0 == null) {
+throw new AssertionError("interpreter returned null for " + v0);
+ }
+ Comparable v1 = RexInterpreter.evaluate(literalOperand,
Collections.emptyMap());
+ if (v1 == null) {
+throw new AssertionError("interpreter returned null for " + v1);
+ }
+ long modifiedVal = (long) v0;
+ final long originalVal = (long) v1;
+ // Since the view contains a FLOOR() if the query does a > or <= and the
operand is already
+ // a FLOORED value we have to shift the value to the next higher or
lower floored value
+ // If the query contains a >= or < we don't need to shift the modified
value
+ if (shiftModifiedVal && modifiedVal == originalVal) {
+final RexLiteral literal = (RexLiteral) floorCall.getOperands().get(1);
+final TimeUnitRange unit =
castNonNull(literal.getValueAs(TimeUnitRange.class));
+if
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020901359
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -1252,6 +1285,193 @@ protected RexNode replaceWithOriginalReferences(final
RexBuilder rexBuilder,
}
}
+ /**
+ * Used to generate a view predicate that is added to a materialized view
that aggregates
+ * over a FLOOR(datetime) when the query has a range predicate on the same
column.
+ */
+ static class ImplicitViewPredicateShuttle extends RexShuttle {
+
+private final RexBuilder rexBuilder;
+private final RexCall floorCall;
+private final RexInputRef rexInputRef;
+private final boolean generateViewFilter;
+private long lowerBound;
+private long upperBound;
+
+ImplicitViewPredicateShuttle(RexBuilder rexBuilder, RexCall floorCall,
+RexInputRef rexInputRef,
+boolean generateViewFilter) {
+ this.floorCall = floorCall;
+ this.rexBuilder = rexBuilder;
+ this.rexInputRef = rexInputRef;
+ this.generateViewFilter = generateViewFilter;
+}
+
+private RexNode transformCall(RexCall call, boolean isLowerBound) {
+ SqlOperator transformedCallOperator = isLowerBound
+ ? SqlStdOperatorTable.GREATER_THAN_OR_EQUAL :
SqlStdOperatorTable.LESS_THAN;
+ // matches functions of the form x > 5 or 5 > x
+ RexNode literalOperand = call.operands.get(0);
+ RexNode tableInputRefOperand = call.operands.get(1);
+ final int floorIndex = ((RexInputRef)
floorCall.getOperands().get(0)).getIndex();
+ boolean reverseOperands = false;
+ if ((literalOperand.getKind() == SqlKind.LITERAL
+ && tableInputRefOperand.getKind() == SqlKind.TABLE_INPUT_REF)
+ || (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+ && tableInputRefOperand.getKind() == SqlKind.LITERAL)) {
+if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+&& tableInputRefOperand.getKind() == SqlKind.LITERAL) {
+ literalOperand = call.operands.get(1);
+ tableInputRefOperand = call.operands.get(0);
+ reverseOperands = true;
+}
+int predicateIndex = ((RexTableInputRef)
tableInputRefOperand).getIndex();
+if (floorIndex == predicateIndex) {
+ // if the query predicate contains a range over the column that is
floored in the
+ // materialized view we can generate a filter on the view
+ boolean shiftTruncatedVal = call.getOperator() !=
transformedCallOperator;
+ long truncatedVal = getModifiedVal(shiftTruncatedVal, isLowerBound,
literalOperand);
+ RexNode truncatedLiteral =
+
rexBuilder.makeTimestampLiteral(TimestampString.fromMillisSinceEpoch(truncatedVal),
+ 0);
+ if (isLowerBound) {
+lowerBound = truncatedVal;
+ } else {
+upperBound = truncatedVal;
+ }
+ if (generateViewFilter) {
+tableInputRefOperand = rexInputRef;
+ }
+ RexNode modifiedCall = rexBuilder.makeCall(call.getType(),
transformedCallOperator,
+ reverseOperands ? ImmutableList.of(tableInputRefOperand,
truncatedLiteral)
+ : ImmutableList.of(truncatedLiteral, tableInputRefOperand));
+ return modifiedCall;
+} else {
+ // ignore predicates on different columns
+ return rexBuilder.makeLiteral(true);
+}
+ }
+ return super.visitCall(call);
+}
+
+private long getModifiedVal(boolean shiftModifiedVal, boolean isLowerBound,
+RexNode literalOperand) {
+ SqlOperator floorOperator = isLowerBound ? SqlStdOperatorTable.CEIL
+ : SqlStdOperatorTable.FLOOR;
+ RexNode truncatedLiteral = rexBuilder.makeCall(floorCall.getType(),
+ floorOperator, ImmutableList.of(literalOperand,
floorCall.getOperands().get(1)));
+ Comparable v0 = RexInterpreter.evaluate(truncatedLiteral,
Collections.emptyMap());
+ if (v0 == null) {
+throw new AssertionError("interpreter returned null for " + v0);
+ }
+ Comparable v1 = RexInterpreter.evaluate(literalOperand,
Collections.emptyMap());
+ if (v1 == null) {
+throw new AssertionError("interpreter returned null for " + v1);
+ }
+ long modifiedVal = (long) v0;
+ final long originalVal = (long) v1;
+ // Since the view contains a FLOOR() if the query does a > or <= and the
operand is already
+ // a FLOORED value we have to shift the value to the next higher or
lower floored value
+ // If the query contains a >= or < we don't need to shift the modified
value
Review Comment:
Can you add an example here?
--
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:
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020901059
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -1252,6 +1285,193 @@ protected RexNode replaceWithOriginalReferences(final
RexBuilder rexBuilder,
}
}
+ /**
+ * Used to generate a view predicate that is added to a materialized view
that aggregates
+ * over a FLOOR(datetime) when the query has a range predicate on the same
column.
+ */
+ static class ImplicitViewPredicateShuttle extends RexShuttle {
+
+private final RexBuilder rexBuilder;
+private final RexCall floorCall;
+private final RexInputRef rexInputRef;
+private final boolean generateViewFilter;
+private long lowerBound;
+private long upperBound;
+
+ImplicitViewPredicateShuttle(RexBuilder rexBuilder, RexCall floorCall,
+RexInputRef rexInputRef,
+boolean generateViewFilter) {
+ this.floorCall = floorCall;
+ this.rexBuilder = rexBuilder;
+ this.rexInputRef = rexInputRef;
+ this.generateViewFilter = generateViewFilter;
+}
+
+private RexNode transformCall(RexCall call, boolean isLowerBound) {
+ SqlOperator transformedCallOperator = isLowerBound
+ ? SqlStdOperatorTable.GREATER_THAN_OR_EQUAL :
SqlStdOperatorTable.LESS_THAN;
+ // matches functions of the form x > 5 or 5 > x
+ RexNode literalOperand = call.operands.get(0);
+ RexNode tableInputRefOperand = call.operands.get(1);
+ final int floorIndex = ((RexInputRef)
floorCall.getOperands().get(0)).getIndex();
+ boolean reverseOperands = false;
+ if ((literalOperand.getKind() == SqlKind.LITERAL
+ && tableInputRefOperand.getKind() == SqlKind.TABLE_INPUT_REF)
+ || (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+ && tableInputRefOperand.getKind() == SqlKind.LITERAL)) {
+if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+&& tableInputRefOperand.getKind() == SqlKind.LITERAL) {
+ literalOperand = call.operands.get(1);
+ tableInputRefOperand = call.operands.get(0);
+ reverseOperands = true;
+}
+int predicateIndex = ((RexTableInputRef)
tableInputRefOperand).getIndex();
+if (floorIndex == predicateIndex) {
+ // if the query predicate contains a range over the column that is
floored in the
+ // materialized view we can generate a filter on the view
+ boolean shiftTruncatedVal = call.getOperator() !=
transformedCallOperator;
+ long truncatedVal = getModifiedVal(shiftTruncatedVal, isLowerBound,
literalOperand);
+ RexNode truncatedLiteral =
+
rexBuilder.makeTimestampLiteral(TimestampString.fromMillisSinceEpoch(truncatedVal),
+ 0);
+ if (isLowerBound) {
+lowerBound = truncatedVal;
+ } else {
+upperBound = truncatedVal;
+ }
+ if (generateViewFilter) {
+tableInputRefOperand = rexInputRef;
+ }
+ RexNode modifiedCall = rexBuilder.makeCall(call.getType(),
transformedCallOperator,
+ reverseOperands ? ImmutableList.of(tableInputRefOperand,
truncatedLiteral)
+ : ImmutableList.of(truncatedLiteral, tableInputRefOperand));
+ return modifiedCall;
+} else {
+ // ignore predicates on different columns
+ return rexBuilder.makeLiteral(true);
+}
+ }
+ return super.visitCall(call);
+}
+
+private long getModifiedVal(boolean shiftModifiedVal, boolean isLowerBound,
+RexNode literalOperand) {
+ SqlOperator floorOperator = isLowerBound ? SqlStdOperatorTable.CEIL
+ : SqlStdOperatorTable.FLOOR;
+ RexNode truncatedLiteral = rexBuilder.makeCall(floorCall.getType(),
+ floorOperator, ImmutableList.of(literalOperand,
floorCall.getOperands().get(1)));
+ Comparable v0 = RexInterpreter.evaluate(truncatedLiteral,
Collections.emptyMap());
+ if (v0 == null) {
+throw new AssertionError("interpreter returned null for " + v0);
+ }
+ Comparable v1 = RexInterpreter.evaluate(literalOperand,
Collections.emptyMap());
+ if (v1 == null) {
+throw new AssertionError("interpreter returned null for " + v1);
+ }
Review Comment:
`v0` and `v1` are always null when you build the assertion message, replace
the value with something that makes sense
Moreover, sentences should start with a capital letter
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020901059
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -1252,6 +1285,193 @@ protected RexNode replaceWithOriginalReferences(final
RexBuilder rexBuilder,
}
}
+ /**
+ * Used to generate a view predicate that is added to a materialized view
that aggregates
+ * over a FLOOR(datetime) when the query has a range predicate on the same
column.
+ */
+ static class ImplicitViewPredicateShuttle extends RexShuttle {
+
+private final RexBuilder rexBuilder;
+private final RexCall floorCall;
+private final RexInputRef rexInputRef;
+private final boolean generateViewFilter;
+private long lowerBound;
+private long upperBound;
+
+ImplicitViewPredicateShuttle(RexBuilder rexBuilder, RexCall floorCall,
+RexInputRef rexInputRef,
+boolean generateViewFilter) {
+ this.floorCall = floorCall;
+ this.rexBuilder = rexBuilder;
+ this.rexInputRef = rexInputRef;
+ this.generateViewFilter = generateViewFilter;
+}
+
+private RexNode transformCall(RexCall call, boolean isLowerBound) {
+ SqlOperator transformedCallOperator = isLowerBound
+ ? SqlStdOperatorTable.GREATER_THAN_OR_EQUAL :
SqlStdOperatorTable.LESS_THAN;
+ // matches functions of the form x > 5 or 5 > x
+ RexNode literalOperand = call.operands.get(0);
+ RexNode tableInputRefOperand = call.operands.get(1);
+ final int floorIndex = ((RexInputRef)
floorCall.getOperands().get(0)).getIndex();
+ boolean reverseOperands = false;
+ if ((literalOperand.getKind() == SqlKind.LITERAL
+ && tableInputRefOperand.getKind() == SqlKind.TABLE_INPUT_REF)
+ || (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+ && tableInputRefOperand.getKind() == SqlKind.LITERAL)) {
+if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+&& tableInputRefOperand.getKind() == SqlKind.LITERAL) {
+ literalOperand = call.operands.get(1);
+ tableInputRefOperand = call.operands.get(0);
+ reverseOperands = true;
+}
+int predicateIndex = ((RexTableInputRef)
tableInputRefOperand).getIndex();
+if (floorIndex == predicateIndex) {
+ // if the query predicate contains a range over the column that is
floored in the
+ // materialized view we can generate a filter on the view
+ boolean shiftTruncatedVal = call.getOperator() !=
transformedCallOperator;
+ long truncatedVal = getModifiedVal(shiftTruncatedVal, isLowerBound,
literalOperand);
+ RexNode truncatedLiteral =
+
rexBuilder.makeTimestampLiteral(TimestampString.fromMillisSinceEpoch(truncatedVal),
+ 0);
+ if (isLowerBound) {
+lowerBound = truncatedVal;
+ } else {
+upperBound = truncatedVal;
+ }
+ if (generateViewFilter) {
+tableInputRefOperand = rexInputRef;
+ }
+ RexNode modifiedCall = rexBuilder.makeCall(call.getType(),
transformedCallOperator,
+ reverseOperands ? ImmutableList.of(tableInputRefOperand,
truncatedLiteral)
+ : ImmutableList.of(truncatedLiteral, tableInputRefOperand));
+ return modifiedCall;
+} else {
+ // ignore predicates on different columns
+ return rexBuilder.makeLiteral(true);
+}
+ }
+ return super.visitCall(call);
+}
+
+private long getModifiedVal(boolean shiftModifiedVal, boolean isLowerBound,
+RexNode literalOperand) {
+ SqlOperator floorOperator = isLowerBound ? SqlStdOperatorTable.CEIL
+ : SqlStdOperatorTable.FLOOR;
+ RexNode truncatedLiteral = rexBuilder.makeCall(floorCall.getType(),
+ floorOperator, ImmutableList.of(literalOperand,
floorCall.getOperands().get(1)));
+ Comparable v0 = RexInterpreter.evaluate(truncatedLiteral,
Collections.emptyMap());
+ if (v0 == null) {
+throw new AssertionError("interpreter returned null for " + v0);
+ }
+ Comparable v1 = RexInterpreter.evaluate(literalOperand,
Collections.emptyMap());
+ if (v1 == null) {
+throw new AssertionError("interpreter returned null for " + v1);
+ }
Review Comment:
`v0` and `v1` are always null when you build the assertion message, replace
the value with something that makes sense
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020900761
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -1252,6 +1285,193 @@ protected RexNode replaceWithOriginalReferences(final
RexBuilder rexBuilder,
}
}
+ /**
+ * Used to generate a view predicate that is added to a materialized view
that aggregates
+ * over a FLOOR(datetime) when the query has a range predicate on the same
column.
+ */
+ static class ImplicitViewPredicateShuttle extends RexShuttle {
+
+private final RexBuilder rexBuilder;
+private final RexCall floorCall;
+private final RexInputRef rexInputRef;
+private final boolean generateViewFilter;
+private long lowerBound;
+private long upperBound;
+
+ImplicitViewPredicateShuttle(RexBuilder rexBuilder, RexCall floorCall,
+RexInputRef rexInputRef,
+boolean generateViewFilter) {
+ this.floorCall = floorCall;
+ this.rexBuilder = rexBuilder;
+ this.rexInputRef = rexInputRef;
+ this.generateViewFilter = generateViewFilter;
+}
+
+private RexNode transformCall(RexCall call, boolean isLowerBound) {
+ SqlOperator transformedCallOperator = isLowerBound
+ ? SqlStdOperatorTable.GREATER_THAN_OR_EQUAL :
SqlStdOperatorTable.LESS_THAN;
+ // matches functions of the form x > 5 or 5 > x
+ RexNode literalOperand = call.operands.get(0);
+ RexNode tableInputRefOperand = call.operands.get(1);
+ final int floorIndex = ((RexInputRef)
floorCall.getOperands().get(0)).getIndex();
+ boolean reverseOperands = false;
+ if ((literalOperand.getKind() == SqlKind.LITERAL
+ && tableInputRefOperand.getKind() == SqlKind.TABLE_INPUT_REF)
+ || (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+ && tableInputRefOperand.getKind() == SqlKind.LITERAL)) {
+if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+&& tableInputRefOperand.getKind() == SqlKind.LITERAL) {
+ literalOperand = call.operands.get(1);
+ tableInputRefOperand = call.operands.get(0);
+ reverseOperands = true;
+}
+int predicateIndex = ((RexTableInputRef)
tableInputRefOperand).getIndex();
+if (floorIndex == predicateIndex) {
Review Comment:
Here we could probably replace the if/else with a call to an aux method
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020900532
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -1252,6 +1285,193 @@ protected RexNode replaceWithOriginalReferences(final
RexBuilder rexBuilder,
}
}
+ /**
+ * Used to generate a view predicate that is added to a materialized view
that aggregates
+ * over a FLOOR(datetime) when the query has a range predicate on the same
column.
+ */
+ static class ImplicitViewPredicateShuttle extends RexShuttle {
+
+private final RexBuilder rexBuilder;
+private final RexCall floorCall;
+private final RexInputRef rexInputRef;
+private final boolean generateViewFilter;
+private long lowerBound;
+private long upperBound;
+
+ImplicitViewPredicateShuttle(RexBuilder rexBuilder, RexCall floorCall,
+RexInputRef rexInputRef,
+boolean generateViewFilter) {
+ this.floorCall = floorCall;
+ this.rexBuilder = rexBuilder;
+ this.rexInputRef = rexInputRef;
+ this.generateViewFilter = generateViewFilter;
+}
+
+private RexNode transformCall(RexCall call, boolean isLowerBound) {
+ SqlOperator transformedCallOperator = isLowerBound
+ ? SqlStdOperatorTable.GREATER_THAN_OR_EQUAL :
SqlStdOperatorTable.LESS_THAN;
+ // matches functions of the form x > 5 or 5 > x
+ RexNode literalOperand = call.operands.get(0);
+ RexNode tableInputRefOperand = call.operands.get(1);
+ final int floorIndex = ((RexInputRef)
floorCall.getOperands().get(0)).getIndex();
+ boolean reverseOperands = false;
+ if ((literalOperand.getKind() == SqlKind.LITERAL
+ && tableInputRefOperand.getKind() == SqlKind.TABLE_INPUT_REF)
+ || (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+ && tableInputRefOperand.getKind() == SqlKind.LITERAL)) {
+if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+&& tableInputRefOperand.getKind() == SqlKind.LITERAL) {
Review Comment:
What about simplifying the if condition by introducing some boolean
variables like `isLeftLiteral`, `isRightLiteral` etc?
```suggestion
final boolean isLeftLiteral = literalOperand.getKind() ==
SqlKind.LITERAL;
final boolean isRightLiteral = tableInputRefOperand.getKind() ==
SqlKind.LITERAL;
final boolean isLeftTableInputRef = literalOperand.getKind() ==
SqlKind.TABLE_INPUT_REF;
final boolean isRightTableInputRef = tableInputRefOperand.getKind() ==
SqlKind.TABLE_INPUT_REF;
boolean reverseOperands = false;
if (isLeftLiteral && isRightTableInputRef || isLeftTableInputRef &&
isRightLiteral) {
if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
&& tableInputRefOperand.getKind() == SqlKind.LITERAL) {
```
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020892400
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -1252,6 +1285,193 @@ protected RexNode replaceWithOriginalReferences(final
RexBuilder rexBuilder,
}
}
+ /**
+ * Used to generate a view predicate that is added to a materialized view
that aggregates
+ * over a FLOOR(datetime) when the query has a range predicate on the same
column.
+ */
+ static class ImplicitViewPredicateShuttle extends RexShuttle {
+
+private final RexBuilder rexBuilder;
+private final RexCall floorCall;
+private final RexInputRef rexInputRef;
+private final boolean generateViewFilter;
+private long lowerBound;
+private long upperBound;
+
+ImplicitViewPredicateShuttle(RexBuilder rexBuilder, RexCall floorCall,
+RexInputRef rexInputRef,
+boolean generateViewFilter) {
+ this.floorCall = floorCall;
+ this.rexBuilder = rexBuilder;
+ this.rexInputRef = rexInputRef;
+ this.generateViewFilter = generateViewFilter;
+}
+
+private RexNode transformCall(RexCall call, boolean isLowerBound) {
Review Comment:
We need to split this method using auxiliary ones, can you rework it to
reduce the overall complexity?
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020892294
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -1252,6 +1285,193 @@ protected RexNode replaceWithOriginalReferences(final
RexBuilder rexBuilder,
}
}
+ /**
+ * Used to generate a view predicate that is added to a materialized view
that aggregates
+ * over a FLOOR(datetime) when the query has a range predicate on the same
column.
+ */
+ static class ImplicitViewPredicateShuttle extends RexShuttle {
+
+private final RexBuilder rexBuilder;
+private final RexCall floorCall;
+private final RexInputRef rexInputRef;
+private final boolean generateViewFilter;
+private long lowerBound;
+private long upperBound;
+
+ImplicitViewPredicateShuttle(RexBuilder rexBuilder, RexCall floorCall,
+RexInputRef rexInputRef,
+boolean generateViewFilter) {
+ this.floorCall = floorCall;
+ this.rexBuilder = rexBuilder;
+ this.rexInputRef = rexInputRef;
+ this.generateViewFilter = generateViewFilter;
+}
+
+private RexNode transformCall(RexCall call, boolean isLowerBound) {
+ SqlOperator transformedCallOperator = isLowerBound
+ ? SqlStdOperatorTable.GREATER_THAN_OR_EQUAL :
SqlStdOperatorTable.LESS_THAN;
+ // matches functions of the form x > 5 or 5 > x
+ RexNode literalOperand = call.operands.get(0);
+ RexNode tableInputRefOperand = call.operands.get(1);
+ final int floorIndex = ((RexInputRef)
floorCall.getOperands().get(0)).getIndex();
+ boolean reverseOperands = false;
+ if ((literalOperand.getKind() == SqlKind.LITERAL
+ && tableInputRefOperand.getKind() == SqlKind.TABLE_INPUT_REF)
+ || (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+ && tableInputRefOperand.getKind() == SqlKind.LITERAL)) {
+if (literalOperand.getKind() == SqlKind.TABLE_INPUT_REF
+&& tableInputRefOperand.getKind() == SqlKind.LITERAL) {
+ literalOperand = call.operands.get(1);
+ tableInputRefOperand = call.operands.get(0);
+ reverseOperands = true;
+}
+int predicateIndex = ((RexTableInputRef)
tableInputRefOperand).getIndex();
+if (floorIndex == predicateIndex) {
+ // if the query predicate contains a range over the column that is
floored in the
+ // materialized view we can generate a filter on the view
+ boolean shiftTruncatedVal = call.getOperator() !=
transformedCallOperator;
+ long truncatedVal = getModifiedVal(shiftTruncatedVal, isLowerBound,
literalOperand);
+ RexNode truncatedLiteral =
+
rexBuilder.makeTimestampLiteral(TimestampString.fromMillisSinceEpoch(truncatedVal),
+ 0);
+ if (isLowerBound) {
+lowerBound = truncatedVal;
+ } else {
+upperBound = truncatedVal;
+ }
+ if (generateViewFilter) {
+tableInputRefOperand = rexInputRef;
+ }
+ RexNode modifiedCall = rexBuilder.makeCall(call.getType(),
transformedCallOperator,
+ reverseOperands ? ImmutableList.of(tableInputRefOperand,
truncatedLiteral)
+ : ImmutableList.of(truncatedLiteral, tableInputRefOperand));
+ return modifiedCall;
Review Comment:
I guess you use the variable for easing debug, for modern debuggers can step
past the return expression, I think we lose clarity and we don't gain much, I
suggest to drop the variable and return immediately.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020891962
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewRule.java:
##
@@ -1252,6 +1285,193 @@ protected RexNode replaceWithOriginalReferences(final
RexBuilder rexBuilder,
}
}
+ /**
+ * Used to generate a view predicate that is added to a materialized view
that aggregates
+ * over a FLOOR(datetime) when the query has a range predicate on the same
column.
+ */
+ static class ImplicitViewPredicateShuttle extends RexShuttle {
+
+private final RexBuilder rexBuilder;
+private final RexCall floorCall;
+private final RexInputRef rexInputRef;
+private final boolean generateViewFilter;
+private long lowerBound;
+private long upperBound;
+
+ImplicitViewPredicateShuttle(RexBuilder rexBuilder, RexCall floorCall,
+RexInputRef rexInputRef,
+boolean generateViewFilter) {
Review Comment:
Nit: we generally don't stack up arguments in Calcite
```suggestion
RexInputRef rexInputRef, boolean generateViewFilter) {
```
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020889953
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewAggregateRule.java:
##
@@ -910,6 +942,53 @@ protected SqlFunction getFloorSqlFunction(TimeUnitRange
flag) {
return Pair.of(resultTopViewProject, requireNonNull(resultViewNode,
"resultViewNode"));
}
+ /**
+ * If the view contains a FLOOR(col) and the query contains a range
predicate on the col then
+ * rewrite the view to include a predicate based on the query predicate so
that the view
+ * can be used.
+ * @return pair of view and added view predicate, or null if the rewrite
can't be done
+ */
+ @Override public @Nullable Pair
rewriteInputView(RelOptRuleCall call,
+ RelNode view, RelNode viewNode, RexBuilder rexBuilder, Pair queryPreds,
+ RexNode viewPred) {
+if (!queryPreds.right.isAlwaysTrue() && viewPred.isAlwaysTrue()) {
+ // If there is no view predicate add one based on the query predicate if
the
+ // view is a rollup ( contains an aggregation that groups by FLOOR )
+ if (viewNode instanceof Aggregate) {
+Aggregate aggregate = (Aggregate) viewNode;
+RelNode input = aggregate.getInput();
+if (input instanceof Project) {
+ Project project = (Project) input;
+ int viewColumnIndex = 0;
+ for (RexNode rexNode : project.getProjects()) {
+if (rexNode instanceof RexCall) {
+ RexCall rexCall = (RexCall) rexNode;
Review Comment:
I feel this is the right spot where an auxiliary method could be extracted,
`rewriteInputView` is too long and does a lot of things, what do you think?
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020889694
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewAggregateRule.java:
##
@@ -910,6 +942,53 @@ protected SqlFunction getFloorSqlFunction(TimeUnitRange
flag) {
return Pair.of(resultTopViewProject, requireNonNull(resultViewNode,
"resultViewNode"));
}
+ /**
+ * If the view contains a FLOOR(col) and the query contains a range
predicate on the col then
+ * rewrite the view to include a predicate based on the query predicate so
that the view
+ * can be used.
+ * @return pair of view and added view predicate, or null if the rewrite
can't be done
+ */
+ @Override public @Nullable Pair
rewriteInputView(RelOptRuleCall call,
+ RelNode view, RelNode viewNode, RexBuilder rexBuilder, Pair queryPreds,
+ RexNode viewPred) {
+if (!queryPreds.right.isAlwaysTrue() && viewPred.isAlwaysTrue()) {
+ // If there is no view predicate add one based on the query predicate if
the
+ // view is a rollup ( contains an aggregation that groups by FLOOR )
+ if (viewNode instanceof Aggregate) {
+Aggregate aggregate = (Aggregate) viewNode;
+RelNode input = aggregate.getInput();
+if (input instanceof Project) {
+ Project project = (Project) input;
+ int viewColumnIndex = 0;
+ for (RexNode rexNode : project.getProjects()) {
+if (rexNode instanceof RexCall) {
+ RexCall rexCall = (RexCall) rexNode;
+ if (rexCall.getKind() == SqlKind.FLOOR) {
+RexInputRef rexInputRef = RexInputRef.of(viewColumnIndex,
view.getRowType());
+ImplicitViewPredicateShuttle viewPredicateShuttle =
+new ImplicitViewPredicateShuttle(rexBuilder, rexCall,
rexInputRef, false);
+ImplicitViewPredicateShuttle viewPredicateShuttle2 =
+new ImplicitViewPredicateShuttle(rexBuilder, rexCall,
rexInputRef, true);
+RexNode viewPredicate =
queryPreds.right.accept(viewPredicateShuttle);
+if (viewPredicateShuttle.isRangeMatched()) {
+ RexNode viewFilter =
queryPreds.right.accept(viewPredicateShuttle2);
Review Comment:
We can move the init of the second shuttle inside the `if`, the only place
where it's used.
```suggestion
RexNode viewPredicate =
queryPreds.right.accept(viewPredicateShuttle);
if (viewPredicateShuttle.isRangeMatched()) {
ImplicitViewPredicateShuttle viewPredicateShuttle2 =
new ImplicitViewPredicateShuttle(rexBuilder, rexCall,
rexInputRef, true);
RexNode viewFilter =
queryPreds.right.accept(viewPredicateShuttle2);
```
Additionally, let's find a more descriptive name than
`viewPredicateShuttle`, something like`viewPredicateShuttleFilterGenerator`?
Possibly let's improve also `viewPredicateShuttle`, with something like
`viewPredicateShuttleRangeMatcher`?
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020888640
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewAggregateRule.java:
##
@@ -910,6 +942,53 @@ protected SqlFunction getFloorSqlFunction(TimeUnitRange
flag) {
return Pair.of(resultTopViewProject, requireNonNull(resultViewNode,
"resultViewNode"));
}
+ /**
+ * If the view contains a FLOOR(col) and the query contains a range
predicate on the col then
+ * rewrite the view to include a predicate based on the query predicate so
that the view
+ * can be used.
+ * @return pair of view and added view predicate, or null if the rewrite
can't be done
+ */
+ @Override public @Nullable Pair
rewriteInputView(RelOptRuleCall call,
+ RelNode view, RelNode viewNode, RexBuilder rexBuilder, Pair queryPreds,
+ RexNode viewPred) {
+if (!queryPreds.right.isAlwaysTrue() && viewPred.isAlwaysTrue()) {
+ // If there is no view predicate add one based on the query predicate if
the
+ // view is a rollup ( contains an aggregation that groups by FLOOR )
Review Comment:
Nit:
```suggestion
// view is a rollup (contains an aggregation that groups by FLOOR)
```
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020888568
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewAggregateRule.java:
##
@@ -910,6 +942,53 @@ protected SqlFunction getFloorSqlFunction(TimeUnitRange
flag) {
return Pair.of(resultTopViewProject, requireNonNull(resultViewNode,
"resultViewNode"));
}
+ /**
+ * If the view contains a FLOOR(col) and the query contains a range
predicate on the col then
+ * rewrite the view to include a predicate based on the query predicate so
that the view
+ * can be used.
+ * @return pair of view and added view predicate, or null if the rewrite
can't be done
+ */
+ @Override public @Nullable Pair
rewriteInputView(RelOptRuleCall call,
+ RelNode view, RelNode viewNode, RexBuilder rexBuilder, Pair queryPreds,
+ RexNode viewPred) {
+if (!queryPreds.right.isAlwaysTrue() && viewPred.isAlwaysTrue()) {
Review Comment:
`queryPreds.right` is nullable here, we need to check for `queryPreds.right
!= null` before invoking a method to avoid NPE
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020888380
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewAggregateRule.java:
##
@@ -910,6 +942,53 @@ protected SqlFunction getFloorSqlFunction(TimeUnitRange
flag) {
return Pair.of(resultTopViewProject, requireNonNull(resultViewNode,
"resultViewNode"));
}
+ /**
+ * If the view contains a FLOOR(col) and the query contains a range
predicate on the col then
+ * rewrite the view to include a predicate based on the query predicate so
that the view
+ * can be used.
+ * @return pair of view and added view predicate, or null if the rewrite
can't be done
+ */
+ @Override public @Nullable Pair
rewriteInputView(RelOptRuleCall call,
Review Comment:
Remove extra whitespace
```suggestion
@Override public @Nullable Pair
rewriteInputView(RelOptRuleCall call,
```
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020888215
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewAggregateRule.java:
##
@@ -910,6 +942,53 @@ protected SqlFunction getFloorSqlFunction(TimeUnitRange
flag) {
return Pair.of(resultTopViewProject, requireNonNull(resultViewNode,
"resultViewNode"));
}
+ /**
+ * If the view contains a FLOOR(col) and the query contains a range
predicate on the col then
Review Comment:
Nit:
```suggestion
* If the view contains FLOOR(col) and the query contains a range
predicate on col then
```
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876:
URL: https://github.com/apache/calcite/pull/2876#discussion_r1020887754
##
core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewAggregateRule.java:
##
@@ -292,10 +298,36 @@
otherCompensationPred)));
// Generate query rewriting.
-RelNode rewrittenPlan = relBuilder
-.push(target)
-.filter(simplify.simplifyUnknownAsFalse(queryCompensationPred))
-.build();
+RelNode rewrittenPlan = null;
+if (pushQueryFilter) {
+ rewrittenPlan = target.accept(
+ // Push the queryCompensationPred down into any filter that is
present in the plan
+ new RelShuttleImpl() {
+@Override public RelNode visit(LogicalFilter filter) {
+ RexNode condition =
+ RexUtil.flatten(rexBuilder,
+ rexBuilder.makeCall(
+ SqlStdOperatorTable.AND,
+ filter.getCondition(),
+ queryCompensationPred));
+ return filter.copy(filter.getTraitSet(), filter.getInput(),
condition);
+}
+
+@Override public RelNode visit(RelNode other) {
+ if (other instanceof RelSubset) {
+RelSubset relSubset = (RelSubset) other;
+return relSubset.getBestOrOriginal().accept(this);
+ } else {
+return visitChildren(other);
+ }
+}
+ });
+} else {
+ rewrittenPlan = relBuilder
+ .push(target)
Review Comment:
I think it's worth extracting the `RelShuttleImpl` to a private class to
improve readability
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] asolimando commented on a diff in pull request #2876: [CALCITE-5240] Enhance MaterializedViewRule so that it applies to rol…
asolimando commented on code in PR #2876: URL: https://github.com/apache/calcite/pull/2876#discussion_r1020887570 ## core/src/main/java/org/apache/calcite/rel/rules/materialize/MaterializedViewAggregateRule.java: ## @@ -292,10 +298,36 @@ otherCompensationPred))); // Generate query rewriting. -RelNode rewrittenPlan = relBuilder -.push(target) -.filter(simplify.simplifyUnknownAsFalse(queryCompensationPred)) -.build(); +RelNode rewrittenPlan = null; Review Comment: Nit: `null` here is redundant and can be omitted -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] libenchao commented on pull request #2868: [CALCITE-5230] Fix PERCENTILE_DISC return type derivation
libenchao commented on PR #2868: URL: https://github.com/apache/calcite/pull/2868#issuecomment-1312638552 > As for PERCENTILE_CONT, based on conversation with Julian in the Jira ticket, there is nothing to be done. I'm not sure about this conclusion, I've checked the SQL standard (2011), 10.9: > If PERCENTILE_CONT is specified, then: > 1) Let ROW0 be the greatest exact numeric value with scale 0 (zero) that is less than or equal > to NVE*(N–1). Let ROWLIT0 be a representing ROW0. > 2) Let ROW1 be the least exact numeric value with scale 0 (zero) that is greater than or equal to NVE*(N–1). Let ROWLIT1 be a representing ROW1. > 3) Let FACTOR be an representing NVE*(N–1)–ROW0. > 4) The result is the result of the > ( WITH TEMPTABLE(X, Y) AS >( SELECT ROW_NUMBER() > OVER (ORDER BY WSP) - 1, TXCOLNAME > FROM TXANAME ) > SELECT CAST ( T0.Y + FACTOR * (T1.Y - T0.Y) AS DT ) FROM TEMPTABLE T0, TEMPTABLE T1 > WHERE T0.ROWNUMBER = ROWLIT0 > AND T1.ROWNUMBER = ROWLIT1 ) We can see that the standard is using `CAST ( T0.Y + FACTOR * (T1.Y - T0.Y) AS DT )` as the result for `PERCENTILE_COUT`, whose type is the same as the sort key. And also I've checked some databases, and found: [oracle](https://docs.oracle.com/cd/B19306_01/server.102/b14200/functions110.htm) and [postgrelsql](https://www.postgresql.org/docs/9.4/functions-aggregate.html) are consistent with the standard. There are some databases, such as [server sql](https://learn.microsoft.com/en-us/sql/t-sql/functions/percentile-cont-transact-sql?view=sql-server-ver16), are declaring the return type which is not consistent with the standard. Hence, IMO, we should make it consistent with the standard by default. Of course if could be extensible for some dialects, if we want to achieve that, maybe the proper place to add the extension is in `RelDataTypeSystem` -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] libenchao commented on a diff in pull request #2929: [CALCITE-5310] JSON_OBJECT in scalar sub-query throws AssertionError
libenchao commented on code in PR #2929:
URL: https://github.com/apache/calcite/pull/2929#discussion_r1020832236
##
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##
@@ -1790,10 +1790,11 @@ private RexNode createCaseExpression(
@Override public RexNode visitLiteral(RexLiteral literal) {
// Use nullIndicator to decide whether to project null.
- // Do nothing if the literal is null.
+ // Do nothing if the literal is null or symbol.
if (!RexUtil.isNull(literal)
Review Comment:
We are using `LEFT` join type for scalar sub-query decorrelating, see here:
https://github.com/apache/calcite/blob/7277e53adea98e7dd8477d5a47e728aca6d8f680/core/src/main/java/org/apache/calcite/rel/rules/SubQueryRemoveRule.java#L136.
Hence, in this place, we'll wrap all expressions into case expression.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] julianhyde commented on a diff in pull request #2965: [CALCITE-5105] Add MEASURE type and AGGREGATE aggregate function
julianhyde commented on code in PR #2965:
URL: https://github.com/apache/calcite/pull/2965#discussion_r1020820936
##
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorUtil.java:
##
@@ -1287,6 +1287,20 @@ private static FlatAggregate flattenRecurse(@Nullable
SqlCall filterCall,
}
}
+ /** Returns whether a select item is a measure. */
+ public static boolean isMeasure(SqlNode selectItem) {
+return getMeasure(selectItem) != null;
+ }
+
+ /** Returns the measure expression if a select item is a measure, null
+ * otherwise.
+ *
+ * For a measure, {@code selectItem} will have the form
+ * {@code AS(MEASURE(exp), alias)} and this method returns {@code exp}. */
+ public static @Nullable SqlNode getMeasure(SqlNode selectItem) {
+return null;
Review Comment:
Yes, it will be extended in
[CALCITE-4496](https://issues.apache.org/jira/browse/CALCITE-4496). I added a
note.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] julianhyde commented on a diff in pull request #2965: [CALCITE-5105] Add MEASURE type and AGGREGATE aggregate function
julianhyde commented on code in PR #2965:
URL: https://github.com/apache/calcite/pull/2965#discussion_r1020820690
##
core/src/main/java/org/apache/calcite/sql/type/ApplySqlType.java:
##
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.sql.type;
+
+import org.apache.calcite.linq4j.Ord;
+import org.apache.calcite.rel.type.RelDataType;
+
+import com.google.common.collect.ImmutableList;
+
+/** Type that applies generic type to type parameters. */
+abstract class ApplySqlType extends AbstractSqlType {
+ protected final ImmutableList types;
+
+ ApplySqlType(SqlTypeName typeName, boolean isNullable,
+ Iterable types) {
+super(typeName, isNullable, null);
+this.types = ImmutableList.copyOf(types);
Review Comment:
It's possible that 0 type arguments makes sense. So I don't think we should
add the check.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] julianhyde commented on a diff in pull request #2965: [CALCITE-5105] Add MEASURE type and AGGREGATE aggregate function
julianhyde commented on code in PR #2965: URL: https://github.com/apache/calcite/pull/2965#discussion_r1020820454 ## site/_docs/reference.md: ## @@ -49,6 +49,13 @@ here to appease testAllFunctionsAreDocumented: | SUCCEEDS | Documented as a period operator | TABLE | Documented as part of FROM syntax | VARIANCE() | In SqlStdOperatorTable, but not fully implemented + +Dialect-specific: + +| C | Function | Reason not documented +|:--|:-- |:- +| c | AGGREGATE(m) | TODO: document; also AS MEASURE Review Comment: Good point. I've added the code 'c' now, but not much explanation of measures at this point. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite-avatica] snuyanzin opened a new pull request, #196: [CALCITE-5379] Upgrade protobuf to 3.21.9
snuyanzin opened a new pull request, #196: URL: https://github.com/apache/calcite-avatica/pull/196 The PR upgrades protobuf version to avoid CVE-2022-3171 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite-avatica] snuyanzin merged pull request #194: [CALCITE-5373] Upgrade bouncycastle to 1.70
snuyanzin merged PR #194: URL: https://github.com/apache/calcite-avatica/pull/194 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] snuyanzin opened a new pull request, #2966: [CALCITE-5374] Upgrade jackson to 2.14.0
snuyanzin opened a new pull request, #2966: URL: https://github.com/apache/calcite/pull/2966 The PR upgrading jackson to 2.14.0 in 2.14.0 `com.fasterxml.jackson.databind.node.ObjectNode#with` became deprecated after https://github.com/FasterXML/jackson-databind/issues/3568 Since currently such warnings are considered as build error's in current gradle configuration, the PR replaces deprecated methods as well -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite-avatica] snuyanzin commented on pull request #195: [CALCITE-5374] Upgrade jackson to 2.14.0
snuyanzin commented on PR #195: URL: https://github.com/apache/calcite-avatica/pull/195#issuecomment-1312563753 It seems the reason is that in 2.14.0 `com.fasterxml.jackson.databind.node.ObjectNode#with` became deprecated https://github.com/FasterXML/jackson-databind/issues/3568 So first need to update it in Calcite and replace deprecated methods and then could be updated for Avatica. -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite-avatica-go] F21 merged pull request #62: [CALCITE-5353] Document new procedure for requesting JIRA accounts and becoming a contributor
F21 merged PR #62: URL: https://github.com/apache/calcite-avatica-go/pull/62 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite-avatica] F21 merged pull request #188: [CALCITE-5353] Document new procedure for requesting JIRA accounts and becoming a contributor
F21 merged PR #188: URL: https://github.com/apache/calcite-avatica/pull/188 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] F21 merged pull request #2959: [CALCITE-5353] Document new procedure for requesting JIRA accounts and becoming a contributor
F21 merged PR #2959: URL: https://github.com/apache/calcite/pull/2959 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2929: [CALCITE-5310] JSON_OBJECT in scalar sub-query throws AssertionError
dssysolyatin commented on code in PR #2929:
URL: https://github.com/apache/calcite/pull/2929#discussion_r1020099005
##
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##
@@ -1790,10 +1790,11 @@ private RexNode createCaseExpression(
@Override public RexNode visitLiteral(RexLiteral literal) {
// Use nullIndicator to decide whether to project null.
- // Do nothing if the literal is null.
+ // Do nothing if the literal is null or symbol.
if (!RexUtil.isNull(literal)
Review Comment:
Do I understand correctly that if I have query like:
```
SELECT (SELECT CARDINALITY(ARRAY[false, true, a.attidentity = 'a']) FROM
UNNEST(ARRAY[1]) as v) as options
FROM UNNEST(ARRAY['a', 'b']) AS a(attidentity);
```
RelDecorrelator will try to wrap true and false into case expression ?
Another question. why do we need to wrap literals into case expression at
all ?
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2965: [CALCITE-5105] Add MEASURE type and AGGREGATE aggregate function
rubenada commented on PR #2965: URL: https://github.com/apache/calcite/pull/2965#issuecomment-1311503006 @julianhyde Thanks for this contribution. Looks good. I have taken a look, and left some minor comments (I'm not very familiar with SQL parser and validator parts though, so I'm probably not the most suited reviewer in there). -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
