[PR] [CALCITE-6210] Cast to VARBINARY causes an assertion failure [calcite]
mihaibudiu opened a new pull request, #3635: URL: https://github.com/apache/calcite/pull/3635 I have also changed the way strings with non-ASCII characters are unparsed since that would lead to the failure of the tests that parse/unparse/reparse the query. This suggests that the previous way to unparse was not preserving the semantics of the original query, so it was incorrect. The documentation never said how strings are converted to BINARY values, so I documented that as well. The semantics I chose matches Postgres and MySQL. -- 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
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
sonarcloud[bot] commented on PR #3522: URL: https://github.com/apache/calcite/pull/3522#issuecomment-1897612999 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3522) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite=3522=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3522=false=true) [97.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3522=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3522=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3522) -- 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
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
sonarcloud[bot] commented on PR #3522: URL: https://github.com/apache/calcite/pull/3522#issuecomment-1897581471 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3522) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite=3522=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3522=false=true) [97.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3522=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3522=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3522) -- 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
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
mihaibudiu commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1456674069
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -13695,6 +13698,43 @@ private static void
checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * https://issues.apache.org/jira/browse/CALCITE-6111;>[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow.
*/
+ @Test public void testOverflow() {
+final SqlOperatorFixture f = fixture();
+f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)",
Byte.MAX_VALUE),
+".* out of range", true);
+f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as smallint)",
Short.MAX_VALUE),
+".* out of range", true);
+// We use a long value because otherwise calcite interprets the literal as
an integer.
+f.checkFails(String.format(Locale.US, "SELECT cast(%d as int)",
Long.MAX_VALUE),
+".* out of range", true);
+
+// Casting a floating point value larger than the maximum allowed value.
+// 1e60 is larger than the largest BIGINT value allowed.
+f.checkFails("SELECT cast(1e60+30 as tinyint)",
+".* out of range", true);
Review Comment:
My last commit does exactly that. It removes one sonarcloud complaint, as
pointed out by @asolimando.
The other sonarcloud complaint is about a complex method which I didn't
really write, but I slightly modified, so I didn't try to reduce the
complexity. This is after all just a test 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
Re: [PR] [CALCITE-5649] Get row count statistics from ReflectiveSchema [calcite]
sonarcloud[bot] commented on PR #3634: URL: https://github.com/apache/calcite/pull/3634#issuecomment-1897329402 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3634) **Quality Gate passed** Kudos, no new issues were introduced! [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3634=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3634=false=true) [82.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3634=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3634=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3634) -- 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
Re: [PR] [CALCITE-5649] Get row count statistics from ReflectiveSchema [calcite]
jduo commented on PR #3634: URL: https://github.com/apache/calcite/pull/3634#issuecomment-1896606898 It looks like many unrelated tests are failing, and it's probably because reporting row counts has the side effect of altering join orderings. I'm thinking to deal with this we conditionally enable row count statistics on tables. -- 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
Re: [PR] [CALCITE-5649] Get row count statistics from ReflectiveSchema [calcite]
jduo commented on PR #3634: URL: https://github.com/apache/calcite/pull/3634#issuecomment-1896497189 This PR is #3154 with open comments resolved and rebased on the current main. -- 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
Re: [PR] [CALCITE-5649] Get row count statistics from ReflectiveSchema [calcite]
mihaibudiu commented on code in PR #3634:
URL: https://github.com/apache/calcite/pull/3634#discussion_r1456362816
##
core/src/test/java/org/apache/calcite/test/ReflectiveSchemaTest.java:
##
@@ -919,4 +922,30 @@ public static class DateColumnSchema {
"EXPR$0=0",
"EXPR$0=null");
}
+
+ /**
+ * Test that the row count statistic can be retrieved from a
+ * ReflectiveSchema.
+ *
+ * @see https://issues.apache.org/jira/browse/CALCITE-5649;>[CALCITE-5649]
+ *
+ * @throws Exception on error
Review Comment:
well, a test does either throw or doesn't
I think this comment is not useful.
--
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
Re: [PR] CALCITE-5649 Produce row count statistics from ReflectiveSchema for array based tables [calcite]
jduo commented on PR #3154: URL: https://github.com/apache/calcite/pull/3154#issuecomment-1896494469 > > @adamkennedy could you please take a look at the checkStyle violations? > > Sure. This is my first attempt to contribute so I was expecting some issues. Done in #3634. -- 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
Re: [PR] CALCITE-5649 Produce row count statistics from ReflectiveSchema for array based tables [calcite]
jduo commented on code in PR #3154:
URL: https://github.com/apache/calcite/pull/3154#discussion_r1456362710
##
core/src/main/java/org/apache/calcite/adapter/java/ReflectiveSchema.java:
##
@@ -235,6 +240,13 @@ private static Enumerable toEnumerable(final Object o) {
"Cannot convert " + o.getClass() + " into a Enumerable");
}
+ private static @Nullable Double toRowCount(final Object o) {
Review Comment:
Done in #3634 .
--
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
Re: [PR] CALCITE-5649 Produce row count statistics from ReflectiveSchema for array based tables [calcite]
jduo commented on code in PR #3154: URL: https://github.com/apache/calcite/pull/3154#discussion_r1456362392 ## core/src/test/java/org/apache/calcite/test/ReflectiveSchemaTest.java: ## @@ -63,9 +65,7 @@ import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; +import static org.junit.jupiter.api.Assertions.*; Review Comment: Completed in #3634 . -- 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
Re: [PR] CALCITE-5649 Produce row count statistics from ReflectiveSchema for array based tables [calcite]
jduo commented on code in PR #3154:
URL: https://github.com/apache/calcite/pull/3154#discussion_r1456362106
##
core/src/main/java/org/apache/calcite/adapter/java/ReflectiveSchema.java:
##
@@ -235,6 +240,13 @@ private static Enumerable toEnumerable(final Object o) {
"Cannot convert " + o.getClass() + " into a Enumerable");
}
+ private static @Nullable Double toRowCount(final Object o) {
+if (o.getClass().isArray()) {
Review Comment:
Completed in #3634 .
--
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
Re: [PR] CALCITE-5649 Produce row count statistics from ReflectiveSchema for array based tables [calcite]
jduo commented on PR #3154: URL: https://github.com/apache/calcite/pull/3154#issuecomment-1896491775 @rubenada @zoudan @libenchao @adamkennedy , I'm completing this PR in #3634 . Please close this 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
[PR] [CALCITE-5649] Get row count statistics from ReflectiveSchema [calcite]
jduo opened a new pull request, #3634: URL: https://github.com/apache/calcite/pull/3634 (from #3154) This PR adds row count statistics for simple reflective schemas with array and Collection field tables. This small enhancement makes it a little easier to write tests for optimizer rules, particularly rules that might behave specially when the row count is 1 or 0. -- 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
Re: [PR] [CALCITE-5649] Get row count statistics from ReflectiveSchema [calcite]
jduo commented on PR #3634: URL: https://github.com/apache/calcite/pull/3634#issuecomment-1896490250 @rubenada @zoudan @libenchao @adamkennedy -- 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
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
snuyanzin commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1456343324
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -13695,6 +13698,43 @@ private static void
checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * https://issues.apache.org/jira/browse/CALCITE-6111;>[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow.
*/
+ @Test public void testOverflow() {
+final SqlOperatorFixture f = fixture();
+f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)",
Byte.MAX_VALUE),
+".* out of range", true);
+f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as smallint)",
Short.MAX_VALUE),
+".* out of range", true);
+// We use a long value because otherwise calcite interprets the literal as
an integer.
+f.checkFails(String.format(Locale.US, "SELECT cast(%d as int)",
Long.MAX_VALUE),
+".* out of range", true);
+
+// Casting a floating point value larger than the maximum allowed value.
+// 1e60 is larger than the largest BIGINT value allowed.
+f.checkFails("SELECT cast(1e60+30 as tinyint)",
+".* out of range", true);
Review Comment:
nit:
since there is already existing
`org.apache.calcite.sql.test.SqlOperatorFixture#OUT_OF_RANGE_MESSAGE` would it
make sense to reuse it 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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
snuyanzin commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1456341289
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -13695,6 +13697,43 @@ private static void
checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * https://issues.apache.org/jira/browse/CALCITE-6111;>[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow.
*/
+ @Test public void testOverflow() {
+final SqlOperatorFixture f = fixture();
+f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)",
Byte.MAX_VALUE),
+".* out of range", true);
Review Comment:
Was able to test, thanks for clarification
--
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
Re: [PR] [CALCITE-6205] Add BITAND_AGG, BITOR_AGG functions (enabled in Snowfl… [calcite]
tanclary commented on code in PR #3630:
URL: https://github.com/apache/calcite/pull/3630#discussion_r1456310518
##
core/src/main/java/org/apache/calcite/sql/fun/SqlBitOpAggFunction.java:
##
@@ -39,7 +39,7 @@ public class SqlBitOpAggFunction extends SqlAggFunction {
//~ Constructors ---
- /** Creates a SqlBitOpAggFunction. */
+ /** Creates a SqlBitOpAggFunction from a SqlKind. */
Review Comment:
I'm indifferent, the standard operators can derive the name from the
`SqlKind`, the library-specific ones cannot so they need the name `specified`.
Whether it's refactored so they can use the same constructor doesn't matter too
much to me
--
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
Re: [PR] [CALCITE-6198] Add TO_TIMESTAMP_LTZ function (enabled in Snowflake li… [calcite]
julianhyde commented on code in PR #3631:
URL: https://github.com/apache/calcite/pull/3631#discussion_r1456238725
##
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##
@@ -1610,6 +1610,36 @@ private static RelDataType
deriveTypeMapFromEntries(SqlOperatorBinding opBinding
OperandTypes.STRING_STRING,
SqlFunctionCategory.TIMEDATE);
+ /** The "TO_TIMESTAMP_LTZ" function returns a Calcite
Review Comment:
It's worth spelling out that a `TIMESTAMP WITH LOCAL TIME ZONE` is stored as
millis since UTC epoch.
Describe which time zone is used for conversion from `DATE` and `TIMESTAMP`.
##
core/src/main/java/org/apache/calcite/sql/SqlKind.java:
##
@@ -457,6 +457,9 @@ public enum SqlKind {
/** {@code TIME_SUB} function (BigQuery). */
TIME_SUB,
+ /** {@code TIMESTAMP} function (BigQuery). */
+ TIMESTAMP,
Review Comment:
not a good name, since TIMESTAMP is a type
##
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##
@@ -1610,6 +1610,36 @@ private static RelDataType
deriveTypeMapFromEntries(SqlOperatorBinding opBinding
OperandTypes.STRING_STRING,
SqlFunctionCategory.TIMEDATE);
+ /** The "TO_TIMESTAMP_LTZ" function returns a Calcite
+ * {@code TIMESTAMP WITH LOCAL TIME ZONE}.
+ * It has the following overloads (Snowflake also
+ * supports a variant and quoted integer overload but
+ * those are not yet supported):
+ *
+ *
+ * {@code TO_TIMESTAMP_LTZ(numeric[, scale)]}
+ * {@code TO_TIMESTAMP_LTZ(date)}
+ * {@code TO_TIMESTAMP_LTZ(datetime)}
Review Comment:
datetime is not a Calcite type
##
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##
@@ -4461,6 +4461,51 @@ public static int time(long timestampMillis, String
timeZone) {
/ (1000L * 1000L)); // milli > micro > nano
}
+ /** SQL {@code TO_TIMESTAMP_LTZ(date)} function
+ * for a date values. */
+ public static long toTimestampLtzDate(int days) {
+return timestamp(days);
+ }
+
+ /** SQL {@code TO_TIMESTAMP_LTZ(timestampSeconds)}
+ * function for long values. */
+ public static long toTimestampLtz(long timestampSeconds) {
+return toTimestampLtz(timestampSeconds, 0);
+ }
+
+ /** SQL {@code TO_TIMESTAMP_LTZ(timestampSeconds)} function
+ * for BigDecimal values. */
+ public static long toTimestampLtz(BigDecimal timestampSeconds) {
+return toTimestampLtz(timestampSeconds.longValue(), 0);
+ }
+
+ /** SQL {@code TO_TIMESTAMP_LTZ(timestampSeconds, scale)}
+ * function for BigDecimal values with a specified scale. */
+ public static long toTimestampLtz(BigDecimal timestampSeconds, int scale) {
+return toTimestampLtz(timestampSeconds.longValue(), scale);
+ }
+
+ /** SQL {@code TO_TIMESTAMP_LTZ(timestampSeconds, scale)}
+ * function for long values with a specified scale. */
Review Comment:
Can you add examples? Does `toTimestampLtz(86_400L, 0)` mean `TIMESTAMP
'1970-01-02 00:00:00'`?
What would `toTimestampLtz(86_400L, 3)` mean?
##
site/_docs/reference.md:
##
@@ -2746,7 +2746,7 @@ In the following:
| h s | FORMAT_NUMBER(value, decimalVal) | Formats the number
*value* like '#,###,###.##', rounded to decimal places *decimalVal*. If
*decimalVal* is 0, the result has no decimal point or fractional part
| h s | FORMAT_NUMBER(value, format) | Formats the number
*value* to MySQL's FORMAT *format*, like '#,###,###.##0.00'
| b | FORMAT_TIME(string, time) | Formats *time*
according to the specified format *string*
-| b | FORMAT_TIMESTAMP(string timestamp) | Formats *timestamp*
according to the specified format *string*
+| b | FORMAT_TIMESTAMP(string, timestamp) | Formats *timestamp*
according to the specified format *string*
Review Comment:
remove space before bar
##
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##
@@ -4461,6 +4461,51 @@ public static int time(long timestampMillis, String
timeZone) {
/ (1000L * 1000L)); // milli > micro > nano
}
+ /** SQL {@code TO_TIMESTAMP_LTZ(date)} function
+ * for a date values. */
+ public static long toTimestampLtzDate(int days) {
+return timestamp(days);
+ }
+
+ /** SQL {@code TO_TIMESTAMP_LTZ(timestampSeconds)}
+ * function for long values. */
+ public static long toTimestampLtz(long timestampSeconds) {
+return toTimestampLtz(timestampSeconds, 0);
+ }
+
+ /** SQL {@code TO_TIMESTAMP_LTZ(timestampSeconds)} function
+ * for BigDecimal values. */
+ public static long toTimestampLtz(BigDecimal timestampSeconds) {
+return toTimestampLtz(timestampSeconds.longValue(), 0);
+ }
+
+ /** SQL {@code TO_TIMESTAMP_LTZ(timestampSeconds, scale)}
+ * function for BigDecimal values with a specified scale. */
+ public static long toTimestampLtz(BigDecimal timestampSeconds, int scale) {
Re: [PR] [CALCITE-6205] Add BITAND_AGG, BITOR_AGG functions (enabled in Snowfl… [calcite]
olivrlee commented on code in PR #3630:
URL: https://github.com/apache/calcite/pull/3630#discussion_r1456279758
##
core/src/main/java/org/apache/calcite/sql/fun/SqlBitOpAggFunction.java:
##
@@ -39,7 +39,7 @@ public class SqlBitOpAggFunction extends SqlAggFunction {
//~ Constructors ---
- /** Creates a SqlBitOpAggFunction. */
+ /** Creates a SqlBitOpAggFunction from a SqlKind. */
Review Comment:
nit: What do you think about changing the previous call sites of this to
call the new function with
`SqlBitOpAggFunction(call.getKind().getName(), call.getKind())`?
--
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
Re: [PR] [CALCITE-6198] Add TO_TIMESTAMP_LTZ function (enabled in Snowflake li… [calcite]
mihaibudiu commented on PR #3631: URL: https://github.com/apache/calcite/pull/3631#issuecomment-1896331624 Seems that the "Error Prone" check is unhappy. -- 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
(calcite) branch main updated: [CALCITE-6162] Add rule(s) to remove joins with constant single tuple relations
This is an automated email from the ASF dual-hosted git repository.
mbudiu pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/calcite.git
The following commit(s) were added to refs/heads/main by this push:
new 6672f7e69b [CALCITE-6162] Add rule(s) to remove joins with constant
single tuple relations
6672f7e69b is described below
commit 6672f7e69b3c7759131b2b74fdd3ed5fbe100587
Author: Hanumath Maduri
AuthorDate: Fri Dec 22 09:38:53 2023 -0800
[CALCITE-6162] Add rule(s) to remove joins with constant single tuple
relations
---
.../java/org/apache/calcite/plan/RelOptRules.java | 5 +
.../java/org/apache/calcite/rel/core/Values.java | 4 +
.../rel/rules/SingleValuesOptimizationRules.java | 397 +
.../java/org/apache/calcite/test/JdbcTest.java | 36 +-
.../org/apache/calcite/test/RelOptRulesTest.java | 104 ++
.../org/apache/calcite/test/RelOptRulesTest.xml| 250 +
core/src/test/resources/sql/join.iq| 151
core/src/test/resources/sql/sub-query.iq | 27 +-
8 files changed, 948 insertions(+), 26 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/plan/RelOptRules.java
b/core/src/main/java/org/apache/calcite/plan/RelOptRules.java
index 816008f34e..aeb7101c64 100644
--- a/core/src/main/java/org/apache/calcite/plan/RelOptRules.java
+++ b/core/src/main/java/org/apache/calcite/plan/RelOptRules.java
@@ -25,6 +25,7 @@ import org.apache.calcite.rel.rules.CoreRules;
import org.apache.calcite.rel.rules.DateRangeRules;
import org.apache.calcite.rel.rules.JoinPushThroughJoinRule;
import org.apache.calcite.rel.rules.PruneEmptyRules;
+import org.apache.calcite.rel.rules.SingleValuesOptimizationRules;
import org.apache.calcite.rel.rules.materialize.MaterializedViewRules;
import com.google.common.collect.ImmutableList;
@@ -106,6 +107,10 @@ public class RelOptRules {
PruneEmptyRules.JOIN_RIGHT_INSTANCE,
PruneEmptyRules.SORT_FETCH_ZERO_INSTANCE,
PruneEmptyRules.EMPTY_TABLE_INSTANCE,
+ SingleValuesOptimizationRules.JOIN_LEFT_INSTANCE,
+ SingleValuesOptimizationRules.JOIN_RIGHT_INSTANCE,
+ SingleValuesOptimizationRules.JOIN_LEFT_PROJECT_INSTANCE,
+ SingleValuesOptimizationRules.JOIN_RIGHT_PROJECT_INSTANCE,
CoreRules.UNION_MERGE,
CoreRules.INTERSECT_MERGE,
CoreRules.MINUS_MERGE,
diff --git a/core/src/main/java/org/apache/calcite/rel/core/Values.java
b/core/src/main/java/org/apache/calcite/rel/core/Values.java
index c16954bd4d..ed71753495 100644
--- a/core/src/main/java/org/apache/calcite/rel/core/Values.java
+++ b/core/src/main/java/org/apache/calcite/rel/core/Values.java
@@ -152,6 +152,10 @@ public abstract class Values extends AbstractRelNode
implements Hintable {
return !isEmpty(values);
}
+ public static boolean isSingleValue(Values values) {
+return values.tuples.size() == 1;
+ }
+
public ImmutableList> getTuples(RelInput input) {
return input.getTuples("tuples");
}
diff --git
a/core/src/main/java/org/apache/calcite/rel/rules/SingleValuesOptimizationRules.java
b/core/src/main/java/org/apache/calcite/rel/rules/SingleValuesOptimizationRules.java
new file mode 100644
index 00..c10504b31e
--- /dev/null
+++
b/core/src/main/java/org/apache/calcite/rel/rules/SingleValuesOptimizationRules.java
@@ -0,0 +1,397 @@
+/*
+ * 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.rel.rules;
+
+import org.apache.calcite.plan.RelOptRule;
+import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelRule;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.core.Join;
+import org.apache.calcite.rel.core.JoinRelType;
+import org.apache.calcite.rel.core.Project;
+import org.apache.calcite.rel.core.Values;
+import org.apache.calcite.rel.logical.LogicalValues;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexInputRef;
+import org.apache.calcite.rex.RexNode;
+import org.apache.calcite.rex.RexShuttle;
+import org.apache.calcite.rex.RexUtil;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import
Re: [PR] [CALCITE-6162] Add rule(s) to remove joins with constant single tuple… [calcite]
mihaibudiu merged PR #3597: URL: https://github.com/apache/calcite/pull/3597 -- 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
Re: [PR] CALCITE-6208 update JSON_VALUE return type inference to make explicit array return types be nullable with nullable elements [calcite]
mihaibudiu commented on code in PR #3633:
URL: https://github.com/apache/calcite/pull/3633#discussion_r1456152715
##
core/src/main/java/org/apache/calcite/sql/fun/SqlJsonValueFunction.java:
##
@@ -54,20 +53,33 @@ public class SqlJsonValueFunction extends SqlFunction {
public SqlJsonValueFunction(String name) {
super(name, SqlKind.OTHER_FUNCTION,
-ReturnTypes.cascade(
-opBinding ->
explicitTypeSpec(opBinding).orElse(getDefaultType(opBinding)),
-SqlTypeTransforms.FORCE_NULLABLE),
+opBinding -> explicitTypeSpec(opBinding)
+.map(relDataType -> deriveExplicitType(opBinding, relDataType))
+.orElseGet(() -> getDefaultType(opBinding)),
null,
OperandTypes.family(
ImmutableList.of(SqlTypeFamily.ANY, SqlTypeFamily.CHARACTER),
ordinal -> ordinal > 1),
SqlFunctionCategory.SYSTEM);
}
+ private static RelDataType deriveExplicitType(SqlOperatorBinding opBinding,
RelDataType type) {
+if (SqlTypeName.ARRAY == type.getSqlTypeName()) {
+ RelDataType elementType =
Objects.requireNonNull(type.getComponentType());
+ RelDataType nullableElementType = deriveExplicitType(opBinding,
elementType);
+ return SqlTypeUtil.createArrayType(
+ opBinding.getTypeFactory(),
+ nullableElementType,
+ true);
+}
+return opBinding.getTypeFactory().createTypeWithNullability(type, true);
+ }
+
/** Returns VARCHAR(2000) as default. */
private static RelDataType getDefaultType(SqlOperatorBinding opBinding) {
final RelDataTypeFactory typeFactory = opBinding.getTypeFactory();
-return typeFactory.createSqlType(SqlTypeName.VARCHAR, 2000);
+final RelDataType baseType =
typeFactory.createSqlType(SqlTypeName.VARCHAR, 2000);
Review Comment:
why 2000? This will truncate results to 2000 characters. why not just
VARCHAR?
##
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##
@@ -11413,6 +11413,14 @@ private void checkCustomColumnResolving(String table) {
expr("json_value('{\"foo\":100}', 'lax $.foo' returning boolean"
+ " default 100 on empty)")
.columnType("BOOLEAN");
+
+expr("json_value('{\"foo\":[100, null, 200]}', 'lax $.foo'"
++ "returning integer array)")
+.columnType("INTEGER ARRAY");
+
+expr("json_value('{\"foo\":[[100, null, 200]]}', 'lax $.foo'"
Review Comment:
I am not familiar with this function. Can it be used when the array element
types are not the same? JSON certainly allows that.
--
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
Re: [PR] [CALCITE-6205] Add BITAND_AGG, BITOR_AGG functions (enabled in Snowfl… [calcite]
tanclary commented on code in PR #3630:
URL: https://github.com/apache/calcite/pull/3630#discussion_r1456155831
##
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##
@@ -1793,7 +1797,7 @@ static class BitOpImplementor extends
StrictAggImplementor {
if (SqlTypeUtil.isBinary(info.returnRelType())) {
start = Expressions.field(null, ByteString.class, "EMPTY");
} else {
-Object initValue = info.aggregation() == BIT_AND ? -1L : 0;
+Object initValue = info.aggregation().kind == SqlKind.BIT_AND ? -1L :
0;
Review Comment:
This change is to make sure "BITAND_AGG" and "BIT_AND" are caught,
previously it was just checking for a specific operator so "BITAND_AGG" was not
going down the correct codepath
--
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
Re: [PR] [CALCITE-6205] Add BITAND_AGG, BITOR_AGG functions (enabled in Snowfl… [calcite]
olivrlee commented on code in PR #3630:
URL: https://github.com/apache/calcite/pull/3630#discussion_r1456153906
##
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##
@@ -1793,7 +1797,7 @@ static class BitOpImplementor extends
StrictAggImplementor {
if (SqlTypeUtil.isBinary(info.returnRelType())) {
start = Expressions.field(null, ByteString.class, "EMPTY");
} else {
-Object initValue = info.aggregation() == BIT_AND ? -1L : 0;
+Object initValue = info.aggregation().kind == SqlKind.BIT_AND ? -1L :
0;
Review Comment:
This change is just to be consistent with the other functions right?
--
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
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
jduo commented on PR #3632: URL: https://github.com/apache/calcite/pull/3632#issuecomment-1896208862 > @jduo Thanks for your PR. > > Just minor suggestion: > > 1. Commit message should start with `[CALCITE-5647]`. > 2. We like adding Java Document and JIRA link before unit test. Done. -- 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
Re: [PR] CALCITE-5647 Use mq.getRowCount(rel) instead of rel.estimateRowCount(mq) [calcite]
jduo commented on code in PR #3632:
URL: https://github.com/apache/calcite/pull/3632#discussion_r1456054855
##
core/src/test/java/org/apache/calcite/rel/metadata/RelMdPopulationSizeTest.java:
##
@@ -0,0 +1,80 @@
+/*
+ * 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.rel.metadata;
+
+import org.apache.calcite.plan.RelOptCluster;
+import org.apache.calcite.plan.hep.HepPlanner;
+import org.apache.calcite.plan.hep.HepProgram;
+import org.apache.calcite.rel.RelNode;
+import org.apache.calcite.rel.logical.LogicalValues;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rel.type.RelDataTypeSystem;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.sql.type.SqlTypeFactoryImpl;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.util.ImmutableBitSet;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.jupiter.api.Test;
+
+import java.math.BigDecimal;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Tests for {@link RelMdPopulationSize}.
+ */
+public class RelMdPopulationSizeTest {
+
+ @Test public void testPopulationSizeFromValues() {
Review Comment:
Done.
--
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
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
asolimando commented on PR #3522: URL: https://github.com/apache/calcite/pull/3522#issuecomment-1895541892 > > After @snuyanzin's comment I feel we need to get to the bottom of the test issue before merging, therefore marking the PR as "request changes" until clarified > > I have made the fixes you suggested. The tests should be run from the class CalciteSqlOperatorTest to exercise the evaluator. Thanks Mihai, the changes and the patch LGTM, can you please address the Sonar violation regarding the repeated occurrence of the string `".* out of range"`? (just define it once at the beginning of the test for better maintainability in case the error messages changes). PS: please don't force-push while the review process is still on-going, it makes reviewers' life more difficult for not much gain, 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
Re: [PR] [CALCITE-6111] Explicit cast from expression to numeric type doesn't check overflow [calcite]
asolimando commented on code in PR #3522:
URL: https://github.com/apache/calcite/pull/3522#discussion_r1455199716
##
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##
@@ -13695,6 +13697,43 @@ private static void
checkLogicalOrFunc(SqlOperatorFixture f) {
}
}
+ /**
+ * Test cases for
+ * https://issues.apache.org/jira/browse/CALCITE-6111;>[CALCITE-6111]
+ * Explicit cast from expression to numeric type doesn't check overflow.
*/
+ @Test public void testOverflow() {
+final SqlOperatorFixture f = fixture();
+f.checkFails(String.format(Locale.US, "SELECT cast(%d+30 as tinyint)",
Byte.MAX_VALUE),
+".* out of range", true);
Review Comment:
Thanks for the refresher Mihai, I always forget about this behaviour in some
of our test classes.
I have verified the tests, they correctly fail if a non-overflowing
value/expression is passed, so they do their job.
--
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
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
sonarcloud[bot] commented on PR #3616: URL: https://github.com/apache/calcite/pull/3616#issuecomment-1895455901 ## [](https://sonarcloud.io/dashboard?id=apache_calcite=3616) **Quality Gate passed** The SonarCloud Quality Gate passed, but some issues were introduced. [3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite=3616=false=true) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite=3616=false=true) [92.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3616=new_coverage=list) [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite=3616=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite=3616) -- 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
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
l4wei commented on PR #3616: URL: https://github.com/apache/calcite/pull/3616#issuecomment-1895388190 > Can you consider reusing existing code as much as possible? > > You just provided a workaround to solve it. I don't think so, I think the real problem is that exploratory replacements have not been undo, so I fix it, this is not a workaround. -- 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
