Add a post-validator visitor that verifies there are no cast to bytes #12475
Conversation
gortiz
force-pushed
the
no-cast-to-bytes
branch
from
b73698f
to
6a70ffa
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12475 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 2376 -60
Lines 133233 129827 -3406
Branches 20636 20138 -498
=============================================
- Hits 82274 0 -82274
- Misses 44911 129827 +84916
+ Partials 6048 0 -6048
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| _originalValidator = originalValidator; | ||
| } | ||
|
|
||
| @SuppressWarnings("checkstyle:WhitespaceAround") |
There was a problem hiding this comment.
(minor) which warning do we violate here?
There was a problem hiding this comment.
^^ Should we remove this?
pinot-query-planner/src/main/java/org/apache/pinot/query/validate/BytesCastVisitor.java
Outdated
Show resolved
Hide resolved
| Preconditions.checkState(sqlNode instanceof SqlDataTypeSpec); | ||
| RelDataType toType = ((SqlDataTypeSpec) sqlNode).deriveType(_originalValidator); | ||
| if (!SqlTypeUtil.isBinary(toType)) { | ||
| return super.visit(call); |
There was a problem hiding this comment.
Same for other short circuit call
| return super.visit(call); | |
| return null; |
There was a problem hiding this comment.
It is not the same, right?
return null just aborts the execution here while return super.visit(call) does in fact return the same value, but has the desired side effect of visiting the operators.
For example, an expression like: CAST (someOperation(CAST ('someliteral' as BINARY)) as STRING) will be converted into:
- CAST
- someOperation
- CAST
- 'someliteral'
- BINARY
- CAST
- STRING
- someOperation
In case we changed this line to return null we won't be able to detect the inner cast to BINARY because we would just abort the visitor once the first CAST to STRING was detected.
| } | ||
| SqlNode srcNode = operands.get(0); | ||
| RelDataType fromType = _originalValidator.getValidatedNodeTypeIfKnown(srcNode); | ||
| if (fromType != null && SqlTypeUtil.isBinary(fromType)) { |
There was a problem hiding this comment.
Will we every cast from binary to binary?
There was a problem hiding this comment.
In Calcite there is BINARY and VARBINARY. In both cases SqlTypeUtil.isBinary return true. I'm assuming we want to allow castings between them.
| message += " Try to wrap the expression in hexToBytes (like hexToBytes(" + srcNode + "))"; | ||
| } | ||
| SqlParserPos pos = call.getParserPosition(); | ||
| RuntimeException ex = new RuntimeException(message); |
There was a problem hiding this comment.
Should we throw a more specific exception?
There was a problem hiding this comment.
Changed to throw a new runtime exception called InvalidCastException
| _originalValidator = originalValidator; | ||
| } | ||
|
|
||
| @SuppressWarnings("checkstyle:WhitespaceAround") |
There was a problem hiding this comment.
^^ Should we remove this?
This PR adds a check that fails if there is an implicit or explicit cast from to Bytes.
I still need to add some automatic tests, but I've tested manually. In a query like:
The message shown is:
From line 0, column 0 to line 2, column 36: Cannot cast '80c062bc98021f94f1404e9bda0f6b0202' as VARBINARY. Try to use binary literal instead (like x'80c062bc98021f94f1404e9bda0f6b0202')In a query like:
The message shown is:
From line 0, column 0 to line 3, column 52: Cannot cast SUBSTRING('80c062bc98021f94f1404e9bda0f6b0202' FROM 2) as VARBINARY. Try to wrap the expression in hexToBytes (like hexToBytes(SUBSTRING('80c062bc98021f94f1404e9bda0f6b0202' FROM 2)))Two things that we can try to improve (although priority doesn't seem high):
Related to #12457