10006 [Planner] Add error in planning for unsupported range types #34926
Conversation
mtabebe
force-pushed
the
ma/10065/range-types
branch
from
a6b5668 to
677fc49
Compare
src/sql/src/plan/typeconv.rs
Outdated
| from: &SqlScalarType, | ||
| to: &SqlScalarType, | ||
| ) -> Option<Cast> { | ||
| ) -> Result<Option<Cast>, PlanError> { |
There was a problem hiding this comment.
I'm wondering about the Result<Option<. The doc comment says
Returns
Errfor casts that are not supported
but this is also true for None returns. Could we collapse this into one level somehow? Maybe by making a new CastError enum, which would have one simple variant (e.g. could be name simply InvalidCast) that covers those things that have been None so far, and would also have more specialized variants, of which there would be only one for now, UnsupportedRangeElementType.
|
|
||
| // Reject casts to range types with unsupported element types at plan time. | ||
| if let SqlScalarType::Range { element_type } = to { | ||
| validate_range_element_type(ecx, element_type)?; |
There was a problem hiding this comment.
I'm wondering what would it take to move this new logic into the VALID_CASTS / CastTemplate code, to have cast validation code be less spread-out. This would need CastTemplate to be able to return finer-grained errors than just None, right? We could add that to it, I guess.
But this would be a somewhat bigger refactoring, so it's ok to consider this out of scope for this PR. But if you agree with the above, could you please add a TODO code comment mentioning it that it would be good to eventually move this range cast validation logic into VALID_CASTS?
(Btw., being able to return more fine-grained errors from CastTemplates would be also useful in another PR that I have open currently #34892, where I'm doing a somewhat hacky thing in the second commit to be able to fish out a specific cast error.)
There was a problem hiding this comment.
My intuition is to separate them into two PRs. One to fix the functional issue and the other as a pure refactoring. What do you think?
Problem: Using an unsupported type with ranges (e.g., float) leads to a panic in either the optimizer or execution: ``` ERROR: internal error: internal error in optimizer: internal transform error: unexpected panic during query optimization: internal error: entered unreachable code: UInt16(1) not yet supported in ranges ``` Solution: Introduce a new error in get_cast that validates the range type (validate_range_element_type) to only the allowed range types (int32, int64, date, numeric, timestamp: https://www.postgresql.org/docs/current/rangetypes.html) Introduce this in get_cast because this is used in the planning phase. If we simply introduce an error in the actual evaluation (e.g., in RangeBound::canonicalize) then it is possible that if the table is empty, an error is not thrown Testing: New tests in range.slt that tests ranges with uint2 and floats.
mtabebe
force-pushed
the
ma/10065/range-types
branch
from
677fc49 to
0fd155e
Compare
2 participants
Problem:
Using an unsupported type with ranges (e.g., float) leads to a panic in either the optimizer or execution:
Solution:
Introduce a new error in get_cast that validates the range type (validate_range_element_type) to only the allowed range types (int32, int64, date, numeric, timestamp: https://www.postgresql.org/docs/current/rangetypes.html)
Introduce this in get_cast because this is used in the planning phase. If we simply introduce an error in the actual evaluation (e.g., in RangeBound::canonicalize) then it is possible that if the table is empty, an error is not thrown
Testing:
New tests in range.slt that tests ranges with uint2 and floats.
Motivation
https://github.com/MaterializeInc/database-issues/issues/10065