-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Add support for default column values in engine #25679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
796b75e
to
9b59d7c
Compare
9b59d7c
to
30ab353
Compare
3eae180
to
2e3b139
Compare
b905a01
to
d75264c
Compare
core/trino-main/src/main/java/io/trino/execution/CreateTableTask.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/execution/CreateTableTask.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/planner/QueryPlanner.java
Outdated
Show resolved
Hide resolved
core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java
Outdated
Show resolved
Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java
Outdated
Show resolved
Hide resolved
@@ -535,7 +535,7 @@ private RelationPlan getInsertPlan( | |||
if (supportsMissingColumnsOnInsert) { | |||
continue; | |||
} | |||
expression = new Constant(column.getType(), null); | |||
expression = new Constant(column.getType(), column.getDefaultValue().orElse(null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the connector supportsMissingColumnsOnInsert
, then it is supposed to apply the default value on its own, right?
Do we have tests to cover both cases (connector applies the default value versus the engine applies it here)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. We don't have such a test. I'm wondering if we can write a helpful test covering both cases in the engine.
d75264c
to
995de6e
Compare
Addressed comments. |
core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java
Outdated
Show resolved
Hide resolved
core/trino-grammar/src/main/antlr4/io/trino/grammar/sql/SqlBase.g4
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/execution/TestCreateTableTask.java
Show resolved
Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-memory/src/test/java/io/trino/plugin/memory/TestMemoryConnectorTest.java
Outdated
Show resolved
Hide resolved
cecbfec
to
62525ca
Compare
Do we plan to support other variants of the default value in the future? For values like |
4a4d211
to
7550565
Compare
I'm a little wondering whether fallback to existing implementation is really good idea. The behavior between default-null and missing-columns might be different in remote connectors. For instance, ClickHouse has default values in each type (e.g. I changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment about type coercion.
@martint could you please check if it makes sense to you? Other than that, I think it's ready for you to review.
core/trino-main/src/main/java/io/trino/util/ColumnDefaultOptions.java
Outdated
Show resolved
Hide resolved
core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java
Outdated
Show resolved
Hide resolved
7550565
to
9507eb0
Compare
core/trino-parser/src/main/java/io/trino/sql/tree/ColumnDefinition.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/util/ColumnDefaultOptions.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/util/ColumnDefaultOptions.java
Outdated
Show resolved
Hide resolved
9507eb0
to
b077120
Compare
Rebased on master (no change). |
9983fbd
to
fad0d2d
Compare
fad0d2d
to
4d85f1f
Compare
@@ -659,6 +701,84 @@ private Query showCreateTable(ShowCreate node) | |||
return singleValueQuery("Create Table", formatSql(createTable).trim()); | |||
} | |||
|
|||
private static Literal toLiteral(Type type, ConnectorExpression expression) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leads me to think that the SPI should take and return a SQL expression (in text form, like we do with row filters and column masks) to represent the default values. Let me ponder this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about translating to IR and building literals with Type.getObjectValue
? It will simplify this method:
io.trino.sql.ir.Constant constant = (io.trino.sql.ir.Constant) ConnectorExpressionTranslator.translate(session, expression, plannerContext, Map.of());
Object value = constant.value();
if (value == null) {
return new NullLiteral();
}
...
case TimestampWithTimeZoneType _ -> new GenericLiteral("TIMESTAMP", type.getObjectValue(null, constant.getValueAsBlock(), 0).toString());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed another commit to show the above idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the issue with this approach is that it requires explicit hardcoded handling of all the types. It doesn't work with extended types (e.g., Geometry / Geography, and others)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about this more, I've come to the conclusion that the default values should be described as SQL expressions, similar to what we do for column masks, row filters and views. Some reasons for this:
- ConnectorExpression is an IR concept, and its type system won't necessarily match the SQL type system over time (especially as we keep evolving and unifying the IR). This will make it harder to describe and recover the SQL-level syntax for operationrs such as DESCRIBE.
- Some of the constructs such as CURRENT_USER, CURRENT_TIMESTAMP, etc. have no correspondence in the IR -- they are desugared during initial planning. It would make it harder to represent them without introducing artificial functions that the connectors need to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Faced some problems during string migration:
- CREAT TABLE and ADD COLUMN: How can we convert evaluated objects to string when passing to SPI? One approach seems casting it as varchar/char type and using the string value. I can't find a better way.
- SHOW CREATE TABLE: We need to convert string to Expression for ColumnDefinition in ShowQueriesRewrite class. Formatting the expression without hard-coded type handling looks still difficult. For instance, I suppose we want to only show
true
orfalse
withoutBOOLEAN '...'
on boolean type. Converting to string in AstBuilder resolves this, but it looks incorrect approach to me.
Pushed new commits with 2 TODO comments in ColumnDefaultOptions and AstBuilder to showcase the above problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we convert evaluated objects to string when passing to SPI
There shouldn't be a need to convert evaluated objects. The expressions should be analyzed during analysis/planning of CREATE TABLE and ADD COLUMN, but then the original string should be passed to the connector for storage.
SHOW CREATE TABLE: We need to convert string to Expression for ColumnDefinition
It should do the same thing SHOW CREATE VIEW does -- parse the expression to create the corresponding AST node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your explanation. Addressed comments.
00de80b
to
d086c12
Compare
Rebased on master to resolve conflicts. |
@@ -1874,6 +1874,7 @@ public static class MergeAnalysis | |||
private final List<List<ColumnHandle>> mergeCaseColumnHandles; | |||
// Case number map to columns | |||
private final Multimap<Integer, ColumnHandle> updateCaseColumnHandles; | |||
private final Map<ColumnHandle, io.trino.sql.ir.Expression> defaultColumnValues; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A question not relates to this : I see that the value is assigned to ir.Expression here. Could you clarify the boundary between when we should use ir.Expression vs. Expression? I’m trying to understand the intended separation or convention
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And does it now actually already to be the Constant
?
@@ -132,6 +137,9 @@ public ListenableFuture<Void> execute( | |||
} | |||
return immediateVoidFuture(); | |||
} | |||
if (element.getDefaultValue().isPresent() && !plannerContext.getMetadata().getConnectorCapabilities(session, catalogHandle).contains(DEFAULT_COLUMN_VALUE)) { | |||
throw semanticException(NOT_SUPPORTED, element, "Catalog '%s' does not support default value for column name '%s'", catalogHandle, columnName); | |||
} | |||
if (!element.isNullable() && !plannerContext.getMetadata().getConnectorCapabilities(session, catalogHandle).contains(NOT_NULL_COLUMN_CONSTRAINT)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when the !element.isNullable()
, what if added defaultValue always is NULL
?
453db51
to
5f36963
Compare
5f36963
to
1c31f22
Compare
1c31f22
to
a7ae149
Compare
Description
This PR adds support for the DEFAULT clause with literal values when creating a new table or adding a column.
The SHOW CREATE TABLE statement now includes the specified default values.
These defaults are respected in INSERT and MERGE operations.
Non-literal expressions such as <datetime value function>, USER, and similar are not supported in this PR.
Release notes