You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "vrajat (via GitHub)" <gi...@apache.org> on 2024/07/18 10:29:03 UTC
[PR] Disable Implicit Cast of Varchar to Int in Multi-Stage engine [pinot]
vrajat opened a new pull request, #13650:
URL: https://github.com/apache/pinot/pull/13650
Apache Calcite allows implicit cast from VARCHAR to INT. The implicit cast may surprise users with a Runtime Exception if some of the rows cannot be parsed or to unexpected results if parsing is unintentionally successful.
As the intention is to keep the multi-stage engine strongly typed, this PR disables implicit casts from VARCHAR to INT.
Postgres also does not allow this implicit cast.
```
postgres=# select * from str;
str
-----
1
a
100
(3 rows)
postgres=# select * from str where str > 10;
ERROR: operator does not exist: character varying > integer
LINE 1: select * from str where str > 10;
^
HINT: No operator matches the given name and argument types. You might need to add explicit type casts.
```
tags: multi-stage, release notes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable Implicit Cast of Varchar to Int in Multi-Stage engine [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat closed pull request #13650: Disable Implicit Cast of Varchar to Int in Multi-Stage engine
URL: https://github.com/apache/pinot/pull/13650
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable Implicit Cast of Varchar to Int in Multi-Stage engine [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #13650:
URL: https://github.com/apache/pinot/pull/13650#issuecomment-2317480084
So... what should we do with this PR? Merge it or close it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable Implicit Cast of Varchar to Int in Multi-Stage engine [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13650:
URL: https://github.com/apache/pinot/pull/13650#discussion_r1736923065
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java:
##########
@@ -1143,9 +1150,17 @@ public void testFunctionWithLiteral(boolean useMultiStageQueryEngine)
testQuery("SELECT SUM(10) FROM mytable");
testQuery("SELECT ArrDelay + 10 FROM mytable");
- testQuery("SELECT ArrDelay + '10' FROM mytable");
+ if (useMultiStageQueryEngine) {
+ testQuery("SELECT ArrDelay + CAST('10' as INT) FROM mytable");
+ } else {
+ testQuery("SELECT ArrDelay + '10' FROM mytable");
Review Comment:
I tried this query and it does work in Postgres
##########
pinot-query-planner/src/test/java/org/apache/pinot/query/validate/PinotTypeCoercionImplTest.java:
##########
@@ -0,0 +1,253 @@
+/**
+ * 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.pinot.query.validate;
+
+import org.apache.pinot.query.QueryEnvironmentTestBase;
+import org.testng.Assert;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+
+public class PinotTypeCoercionImplTest extends QueryEnvironmentTestBase {
+
+ @DataProvider(name = "implicitCastCharToIntInWhere")
+ protected Object[][] provideQueriesWithWhere() {
+ return new Object[][]{
+ // VARCHAR BINARY_COMPARISON INTEGER
+ new Object[]{"SELECT * FROM a where col1 > 5"},
+ new Object[]{"SELECT * FROM a where col1 >= 5"},
+ new Object[]{"SELECT * FROM a where col1 < 5"},
+ new Object[]{"SELECT * FROM a where col1 <= 5"},
+ new Object[]{"SELECT * FROM a where col1 = 5"},
+ new Object[]{"SELECT * FROM a where col1 != 5"},
+
+
+ // INTEGER BINARY_COMPARISON VARCHAR
+ new Object[]{"SELECT * FROM a where 5 > col1"},
+ new Object[]{"SELECT * FROM a where 5 >= col1"},
+ new Object[]{"SELECT * FROM a where 5 < col1"},
+ new Object[]{"SELECT * FROM a where 5 <= col1"},
+ new Object[]{"SELECT * FROM a where 5 = col1"},
+ new Object[]{"SELECT * FROM a where 5 != col1"},
+
+ // (VARCHAR ARITHMETIC_OP INTEGER)
+ new Object[]{"SELECT * FROM a where col1 + 1 > 5"},
+ new Object[]{"SELECT * FROM a where col1 - 1 > 5"},
+ new Object[]{"SELECT * FROM a where col1 * 1 > 5"},
+ new Object[]{"SELECT * FROM a where col1 / 1 > 5"},
+
+ // (INTEGER ARITHMETIC_OP VARCHAR)
+ new Object[]{"SELECT * FROM a where 5 > col1 + 1"},
+ new Object[]{"SELECT * FROM a where 5 > col1 - 1"},
+ new Object[]{"SELECT * FROM a where 5 > col1 * 1"},
+ new Object[]{"SELECT * FROM a where 5 > col1 / 1"},
+
+ // Constants
+ new Object[]{"SELECT * FROM a where '10' > 5"},
Review Comment:
This query also works in Postgres
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java:
##########
@@ -517,7 +517,11 @@ public void testQueryExceptions()
useMultiStageQueryEngine()
? QueryException.QUERY_PLANNING_ERROR_CODE : QueryException.QUERY_EXECUTION_ERROR_CODE);
- testQueryException("SELECT COUNT(*) FROM mytable where ArrTime = 'potato'",
+ // Implicit cast VARCHAR to INT is disabled in V2. So test them separately.
+ String queryV1 = "SELECT COUNT(*) FROM mytable where ArrTime = 'potato'";
Review Comment:
`SELECT COUNT(*) FROM mytable where ArrTime = '10'` works in Postgres
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable Implicit Cast of Varchar to Int in Multi-Stage engine [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on PR #13650:
URL: https://github.com/apache/pinot/pull/13650#issuecomment-2325998240
Lets abandon. I'll add implicit tests in `pg-test-suite` project and then we can take a call based on the results of the compatibility tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable Implicit Cast of Varchar to Int in Multi-Stage engine [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13650:
URL: https://github.com/apache/pinot/pull/13650#discussion_r1736922050
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java:
##########
@@ -517,7 +517,11 @@ public void testQueryExceptions()
useMultiStageQueryEngine()
? QueryException.QUERY_PLANNING_ERROR_CODE : QueryException.QUERY_EXECUTION_ERROR_CODE);
- testQueryException("SELECT COUNT(*) FROM mytable where ArrTime = 'potato'",
+ // Implicit cast VARCHAR to INT is disabled in V2. So test them separately.
+ String queryV1 = "SELECT COUNT(*) FROM mytable where ArrTime = 'potato'";
Review Comment:
`SELECT COUNT(*) FROM mytable where ArrTime = '10'` works in Postgres
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable Implicit Cast of Varchar to Int in Multi-Stage engine [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #13650:
URL: https://github.com/apache/pinot/pull/13650#issuecomment-2236240164
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/13650?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `32 lines` in your changes missing coverage. Please review.
> Project coverage is 27.72%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`db5a17d`)](https://app.codecov.io/gh/apache/pinot/commit/db5a17da69d62a727c10fc9cc471f3ce9e663ee6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 782 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/13650?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...he/pinot/query/validate/PinotTypeCoercionImpl.java](https://app.codecov.io/gh/apache/pinot/pull/13650?src=pr&el=tree&filepath=pinot-query-planner%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fvalidate%2FPinotTypeCoercionImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvdmFsaWRhdGUvUGlub3RUeXBlQ29lcmNpb25JbXBsLmphdmE=) | 0.00% | [29 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13650?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ava/org/apache/pinot/query/validate/Validator.java](https://app.codecov.io/gh/apache/pinot/pull/13650?src=pr&el=tree&filepath=pinot-query-planner%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fvalidate%2FValidator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvdmFsaWRhdGUvVmFsaWRhdG9yLmphdmE=) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13650?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (db5a17d). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (db5a17d) |
>|------|------|------|
>|integration|7|0|
>|integration2|3|0|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|0|
>|unittests|5|1|
>|unittests1|2|0|
>|java-11|5|0|
>|unittests2|3|1|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #13650 +/- ##
=============================================
- Coverage 61.75% 27.72% -34.03%
+ Complexity 207 192 -15
=============================================
Files 2436 2554 +118
Lines 133233 140500 +7267
Branches 20636 21858 +1222
=============================================
- Hits 82274 38958 -43316
- Misses 44911 98553 +53642
+ Partials 6048 2989 -3059
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/13650/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/13650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/13650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/13650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/13650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/13650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/13650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.72% <0.00%> (-33.90%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.72% <0.00%> (+<0.01%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/13650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.72% <0.00%> (-34.03%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/13650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.72% <0.00%> (-34.02%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/13650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/13650/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.72% <0.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/13650?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable Implicit Cast of Varchar to Int in Multi-Stage engine [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on PR #13650:
URL: https://github.com/apache/pinot/pull/13650#issuecomment-2241466108
> What is the postgreSQL behavior on this query? This is backward incompatible, and we need to be careful here
Postgres behaviour is described in the description. This PR makes it similar to PG. Some more info on my investigation:
Implicit conversion table of Calcite is in [SQL language](https://calcite.apache.org/docs/reference.html#implicit-type-conversion) It specifically states that string → int is an implicit conversion. This is different from PG. Char/Varchar → integer requires an explicit cast. The implicit conversions in PG can be seen with the following query:
```
SELECT casttarget::regtype AS target_type
, castsource::regtype AS source_type
, castcontext
FROM pg_cast
WHERE castcontext IN ('i', 'a')
ORDER BY 1, 2;
```
I agree about being careful on breaking backward compatibility. One option is to add a flag for changes that make multi-stage strongly-typed. Has there been any discussions on flags to help move towards ANSI/PG SQL compatability?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable Implicit Cast of Varchar to Int in Multi-Stage engine [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #13650:
URL: https://github.com/apache/pinot/pull/13650#issuecomment-2241325376
What is the postgreSQL behavior on this query?
This is backward incompatible, and we need to be careful 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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable Implicit Cast of Varchar to Int in Multi-Stage engine [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13650:
URL: https://github.com/apache/pinot/pull/13650#discussion_r1685811439
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java:
##########
@@ -558,7 +558,7 @@ public void testBase64Func()
// non-string identifier used in a filter
sqlQuery = "SELECT fromUtf8(fromBase64(toBase64(toUtf8(AirlineID)))), AirlineID FROM mytable WHERE "
- + "fromUtf8(fromBase64(toBase64(toUtf8(AirlineID)))) = AirlineID LIMIT 10";
+ + "fromUtf8(fromBase64(toBase64(toUtf8(AirlineID)))) = CAST(AirlineID as VARCHAR) LIMIT 10";
Review Comment:
We allow implicit casting within UDF (same as v1) here, so it might cause surprise when the left hand doesn't require type casting and right hand requires.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/validate/PinotTypeCoercionImpl.java:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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.pinot.query.validate;
+
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.sql.SqlCallBinding;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.type.SqlTypeUtil;
+import org.apache.calcite.sql.validate.SqlValidator;
+import org.apache.calcite.sql.validate.implicit.TypeCoercionImpl;
+import org.apache.calcite.util.Util;
+
+
+public class PinotTypeCoercionImpl extends TypeCoercionImpl {
Review Comment:
Please add some javadoc describing the difference between this custom type coercion vs the default 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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable Implicit Cast of Varchar to Int in Multi-Stage engine [pinot]
Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on code in PR #13650:
URL: https://github.com/apache/pinot/pull/13650#discussion_r1683802685
##########
pinot-query-planner/src/test/java/org/apache/pinot/query/validate/PinotTypeCoercionImplTest.java:
##########
@@ -0,0 +1,184 @@
+/**
+ * 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.pinot.query.validate;
+
+import org.apache.pinot.query.QueryEnvironmentTestBase;
+import org.testng.Assert;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+
+public class PinotTypeCoercionImplTest extends QueryEnvironmentTestBase {
+
+ @DataProvider(name = "implicitCastCharToIntInWhere")
+ protected Object[][] provideQueriesWithWhere() {
Review Comment:
Please also add examples where two constants are casted. Like `where '10' > 5`. There is where we had issues when trying to remove the coercion from string to binary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable Implicit Cast of Varchar to Int in Multi-Stage engine [pinot]
Posted by "vrajat (via GitHub)" <gi...@apache.org>.
vrajat commented on code in PR #13650:
URL: https://github.com/apache/pinot/pull/13650#discussion_r1684139728
##########
pinot-query-planner/src/test/java/org/apache/pinot/query/validate/PinotTypeCoercionImplTest.java:
##########
@@ -0,0 +1,184 @@
+/**
+ * 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.pinot.query.validate;
+
+import org.apache.pinot.query.QueryEnvironmentTestBase;
+import org.testng.Assert;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+
+public class PinotTypeCoercionImplTest extends QueryEnvironmentTestBase {
+
+ @DataProvider(name = "implicitCastCharToIntInWhere")
+ protected Object[][] provideQueriesWithWhere() {
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Disable Implicit Cast of Varchar to Int in Multi-Stage engine [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13650:
URL: https://github.com/apache/pinot/pull/13650#discussion_r1736922050
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTestSet.java:
##########
@@ -517,7 +517,11 @@ public void testQueryExceptions()
useMultiStageQueryEngine()
? QueryException.QUERY_PLANNING_ERROR_CODE : QueryException.QUERY_EXECUTION_ERROR_CODE);
- testQueryException("SELECT COUNT(*) FROM mytable where ArrTime = 'potato'",
+ // Implicit cast VARCHAR to INT is disabled in V2. So test them separately.
+ String queryV1 = "SELECT COUNT(*) FROM mytable where ArrTime = 'potato'";
Review Comment:
`SELECT COUNT(*) FROM mytable where ArrTime = '10'` works in Postgres
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org