You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by "jnturton (via GitHub)" <gi...@apache.org> on 2023/04/17 11:56:53 UTC
[PR] DRILL-8424: Accommodate RexBuilder changes made for SAFE_CAST (drill)
jnturton opened a new pull request, #2794:
URL: https://github.com/apache/drill/pull/2794
# [DRILL-8424](https://issues.apache.org/jira/browse/DRILL-8424): Accommodate RexBuilder changes made for SAFE_CAST
## Description
Resolves the current CI test failues affecting decimal and empty-literal-to-null casting. Also incorporates upstream syntax additions in Drill's Parser.jj which can only be dropped when [CALCITE-5579](https://issues.apache.org/jira/browse/CALCITE-5579) is resolved.
* Incorporate method signature changes in RexBuilder made by CALCITE-5557.
* Fix float rounding error in TestCastFunctions.testCastFloatDecimalOverflow.
* Incorporate Calcite parser changes.
* [CALCITE-5557] Add SAFE_CAST function (enabled in BigQuery library)
* [CALCITE-5548] Add MSSQL-style CONVERT function (enabled in MSSql library)
* [CALCITE-5554] In EXTRACT function, add DAYOFWEEK and DAYOFYEAR as synonyms for DOW, DOY
* Ignore .mvn/maven.config.
* Upgrade Apache RAT plugin.
* Upgrade os-maven-plugin.
* Move distro tarball to the Maven install phase.
## Documentation
SAFE_CAST to be documented and relevant syntax additions to be documented.
## Testing
Failing tests now pass.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8424: Accommodate RexBuilder changes made for SAFE_CAST (drill)
Posted by "cgivre (via GitHub)" <gi...@apache.org>.
cgivre commented on code in PR #2794:
URL: https://github.com/apache/drill/pull/2794#discussion_r1168893372
##########
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/conversion/DrillRexBuilder.java:
##########
@@ -65,9 +65,9 @@ public RexNode ensureType(
* @return Call to CAST operator
*/
@Override
- public RexNode makeCast(RelDataType type, RexNode exp, boolean matchNullability) {
+ public RexNode makeCast(RelDataType type, RexNode exp, boolean matchNullability, boolean safe) {
Review Comment:
🤦
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8424: Accommodate RexBuilder changes made for SAFE_CAST (drill)
Posted by "jnturton (via GitHub)" <gi...@apache.org>.
jnturton commented on code in PR #2794:
URL: https://github.com/apache/drill/pull/2794#discussion_r1168859598
##########
exec/java-exec/src/main/codegen/templates/Parser.jj:
##########
@@ -7727,6 +7764,8 @@ SqlPostfixOperator PostfixRowOperator() :
| < DATETIME_INTERVAL_CODE: "DATETIME_INTERVAL_CODE" >
| < DATETIME_INTERVAL_PRECISION: "DATETIME_INTERVAL_PRECISION" >
| < DAY: "DAY" >
+| < DAYOFWEEK: "DAYOFWEEK" >
+| < DAYOFYEAR: "DAYOFYEAR" >
Review Comment:
Yes we should, thanks, 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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8424: Accommodate RexBuilder changes made for SAFE_CAST (drill)
Posted by "jnturton (via GitHub)" <gi...@apache.org>.
jnturton commented on PR #2794:
URL: https://github.com/apache/drill/pull/2794#issuecomment-1511297568
I botched the "Move distro tarball to the Maven install phase" commit but that's a one-liner and unrelated to any unit tests so I'll let the test tuns here complete before pushing its fix.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8424: Accommodate RexBuilder changes made for SAFE_CAST (drill)
Posted by "cgivre (via GitHub)" <gi...@apache.org>.
cgivre merged PR #2794:
URL: https://github.com/apache/drill/pull/2794
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8424: Accommodate RexBuilder changes made for SAFE_CAST (drill)
Posted by "cgivre (via GitHub)" <gi...@apache.org>.
cgivre commented on code in PR #2794:
URL: https://github.com/apache/drill/pull/2794#discussion_r1168776897
##########
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/conversion/DrillRexBuilder.java:
##########
@@ -65,9 +65,9 @@ public RexNode ensureType(
* @return Call to CAST operator
*/
@Override
- public RexNode makeCast(RelDataType type, RexNode exp, boolean matchNullability) {
+ public RexNode makeCast(RelDataType type, RexNode exp, boolean matchNullability, boolean safe) {
Review Comment:
This really highlights an issue with Calcite. They really could have added an additional function something like below and nothing would have broken...
```
makeCast(RelDataType type, RexNode exp, boolean matchNullability) {
return makeCast(type, exp, matchNullability, false);
}
```
##########
exec/java-exec/src/main/codegen/templates/Parser.jj:
##########
@@ -15,9 +15,11 @@
* limitations under the License.
*/
-// TODO: Delete this file to reinstate its extraction from calcite-core.jar
-// once CALCITE-5579 is resolved and the incompatible grammar changes introduced
-// by CALCITE-5469 have been backed out. Also see: exec/java-exec/pom.xml.
Review Comment:
Do we want to leave the original info here just so that we know which Calcite PRs we're waiting for?
##########
exec/java-exec/src/main/codegen/templates/Parser.jj:
##########
@@ -7727,6 +7764,8 @@ SqlPostfixOperator PostfixRowOperator() :
| < DATETIME_INTERVAL_CODE: "DATETIME_INTERVAL_CODE" >
| < DATETIME_INTERVAL_PRECISION: "DATETIME_INTERVAL_PRECISION" >
| < DAY: "DAY" >
+| < DAYOFWEEK: "DAYOFWEEK" >
+| < DAYOFYEAR: "DAYOFYEAR" >
Review Comment:
Should we add a unit test for these synonyms?
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8424: Accommodate RexBuilder changes made for SAFE_CAST (drill)
Posted by "jnturton (via GitHub)" <gi...@apache.org>.
jnturton commented on code in PR #2794:
URL: https://github.com/apache/drill/pull/2794#discussion_r1168857297
##########
exec/java-exec/src/main/codegen/templates/Parser.jj:
##########
@@ -15,9 +15,11 @@
* limitations under the License.
*/
-// TODO: Delete this file to reinstate its extraction from calcite-core.jar
-// once CALCITE-5579 is resolved and the incompatible grammar changes introduced
-// by CALCITE-5469 have been backed out. Also see: exec/java-exec/pom.xml.
Review Comment:
Thanks, resolved.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] DRILL-8424: Accommodate RexBuilder changes made for SAFE_CAST (drill)
Posted by "jnturton (via GitHub)" <gi...@apache.org>.
jnturton commented on code in PR #2794:
URL: https://github.com/apache/drill/pull/2794#discussion_r1168863776
##########
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/conversion/DrillRexBuilder.java:
##########
@@ -65,9 +65,9 @@ public RexNode ensureType(
* @return Call to CAST operator
*/
@Override
- public RexNode makeCast(RelDataType type, RexNode exp, boolean matchNullability) {
+ public RexNode makeCast(RelDataType type, RexNode exp, boolean matchNullability, boolean safe) {
Review Comment:
They did do this and only deprecated the original method so our build wasn't broken but our subclass DrillRexBuilder was broken in terms of runtime logic because our method override no longer took effect when it needed to.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org