INSERT/REPLACE complex target column types are validated against source input expressions#16223
Merged
zachjsh merged 13 commits intoapache:masterfrom Apr 16, 2024
Merged
Conversation
10 tasks
kgyrtkirk
reviewed
Apr 12, 2024
| catch (Types.IncompatibleTypeException e) { | ||
| incompatible = true; | ||
| } | ||
| if (incompatible) { |
Member
There was a problem hiding this comment.
- throw a
Types.IncompatibleTypeExceptionexception based on the result of equals - you could place these things inside the
catch - remove the
incompatibleboolean
Contributor
Author
There was a problem hiding this comment.
Good suggestion, fixed.
Comment on lines
479
to
494
| RelDataType relType; | ||
| if (sqlTypeName != null) { | ||
| relType = typeFactory.createSqlType(sqlTypeName); | ||
| } else { | ||
| fields.add(Pair.of( | ||
| colName, | ||
| typeFactory.createTypeWithNullability(relType, true) | ||
| )); | ||
| ColumnType columnType = ColumnType.fromString(definedCol.sqlStorageType()); | ||
| if (columnType != null && columnType.getType().equals(ValueType.COMPLEX)) { | ||
| relType = RowSignatures.makeComplexType(typeFactory, columnType, sourceField.getType().isNullable()); | ||
| } else { | ||
| relType = RowSignatures.columnTypeToRelDataType( | ||
| typeFactory, | ||
| columnType, | ||
| // this nullability is ignored for complex types for some reason, hence the check for complex above. | ||
| sourceField.getType().isNullable() | ||
| ); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
this seems to me that it tries to compute the RelDataType for definedCol ; can we have method or something for that? maybe that could even make it easier to write more direct tests
10 tasks
kgyrtkirk
approved these changes
Apr 16, 2024
abhishekrb19
approved these changes
Apr 16, 2024
Contributor
abhishekrb19
left a comment
abhishekrb19
left a comment
There was a problem hiding this comment.
LGTM. Left a few suggestions that can be done in a follow up.
| .authentication(CalciteTests.SUPER_USER_AUTH_RESULT) | ||
| .expectValidationError( | ||
| DruidException.class, | ||
| StringUtils.format("Operation [%s] requires a PARTITIONED BY to be explicitly defined, but none was found.", operationName) |
Contributor
There was a problem hiding this comment.
Perhaps be more explicit in the error message (the actual code also needs to change):
Suggested change
| StringUtils.format("Operation [%s] requires a PARTITIONED BY to be explicitly defined, but none was found.", operationName) | |
| StringUtils.format("Operation [%s] requires a PARTITIONED BY clause to be explicitly defined in the query or the catalog, but none was found.", operationName) |
| * validation error. | ||
| */ | ||
| @Test | ||
| public void testInsertNoPartitonedByFromCatalog() |
Contributor
There was a problem hiding this comment.
typo:
Suggested change
| public void testInsertNoPartitonedByFromCatalog() | |
| public void testErrorWhenNoPartitionedBy() |
| * value from the catalog. | ||
| */ | ||
| @Test | ||
| public void testInsertHourGrainPartitonedByFromCatalog() |
Contributor
There was a problem hiding this comment.
typo:
Suggested change
| public void testInsertHourGrainPartitonedByFromCatalog() | |
| public void testUsePartitionedByFromCatalog() |
| * the query value is used. | ||
| */ | ||
| @Test | ||
| public void testInsertHourGrainWithDayPartitonedByFromQuery() |
Contributor
There was a problem hiding this comment.
Suggested change
| public void testInsertHourGrainWithDayPartitonedByFromQuery() | |
| public void testUsePartitionedByFromQuery() |
| * the query value is used. | ||
| */ | ||
| @Test | ||
| public void testInsertNoPartitonedByWithDayPartitonedByFromQuery() |
Contributor
There was a problem hiding this comment.
Suggested change
| public void testInsertNoPartitonedByWithDayPartitonedByFromQuery() | |
| public void testUsePartitionedByFromQuery() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This change allows for tables that are defined in the catalog to have their complex defined column types validated against source input expressions mapped to them during DML INSERT/REPLACE operations. The catalog dml unit tests have also been greatly simplified, and a lot of duplicate test code simplified or removed.
This PR has: