Move testInsertIntoNotNullColumn to base connector test #10207
Conversation
There was a problem hiding this comment.
I think the intention of the test originally was to verify that if a CREATE TABLE issued from Trino had NOT NULL columns then writiing NULL to those columns would fail (by the engine, not the remote database). So the error message should be from the engine like NULL value not allowed for NOT NULL column: <column_name>. See https://github.com/trinodb/trino/pull/4144/files#diff-17c56008a5b3b294ad78fca3ce68faef643ff3eea9c5a512fca8e2c753d1d09bR266-R268
This change instead looks to be testing the behaviour of remote database regarding NULL values being inserted into NOT NULL columns.
EDIT: After reading the base test impl I can see the difference.
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
Show resolved
Hide resolved
testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
Show resolved
Hide resolved
ebyhr
force-pushed
the
ebi/test-insert-not-null
branch
2 times, most recently
from
ae23e56 to
70c0cec
Compare
ebyhr
force-pushed
the
ebi/test-insert-not-null
branch
from
70c0cec to
76eb5b4
Compare
ebyhr
force-pushed
the
ebi/test-insert-not-null
branch
from
76eb5b4 to
3c3af03
Compare
| SUPPORTS_RENAME_MATERIALIZED_VIEW_ACROSS_SCHEMAS(SUPPORTS_RENAME_MATERIALIZED_VIEW), | ||
|
|
||
| SUPPORTS_INSERT, | ||
| SUPPORTS_INSERT_NOT_NULL_COLUMN(SUPPORTS_INSERT), |
There was a problem hiding this comment.
call it SUPPORTS_NOT_NULL_CONSTRAINT
There was a problem hiding this comment.
That's misleading (and the cause for my earlier confusion). The constraint is applied by the engine and all connectors "support" it. This is about more about whether the connector allows missing columns on insert (so that remote database replaces missing columns with default values).
Maybe SUPPORTS_DEFAULT_VALUES_FOR_MISSING_COLUMNS or SUPPORTS_MISSING_COLUMNS_ON_INSERT?
There was a problem hiding this comment.
The constraint is applied by the engine and all connectors "support" it
it is applied by the engine, if the connector declares the column as NOT NULL.
It may fail to update io.trino.spi.connector.ColumnMetadata#nullable.
Also, in case io.trino.spi.connector.ConnectorMetadata#supportsMissingColumnsOnInsert, the engine does not enforce NOT NULLs, right?
This is about more about whether the connector allows missing columns on insert
This should be invisible from end user perspective, so we shouldn't ned a TestingConnectorBehavior for this.
so that remote database replaces missing columns with default values).
isn't this related to (or covered by) io.trino.testing.AbstractTestDistributedQueries#testInsertForDefaultColumn?
There was a problem hiding this comment.
I re-read both the tests (testInsertForDefaultColumn and the one we moved here). It's slightly confusing that missing columns on insert is tested as part of testInsertIntoNotNullColumn. Maybe we should remove that (since it's covered by testInsertForDefaultColumn already).
Agreed on all other points.
cc: @ebyhr
There was a problem hiding this comment.
It's slightly confusing that missing columns on insert is tested as part of testInsertIntoNotNullColumn.
i may agree.
It's important to test INSERT with NOT NULL with explicit values and defaults, but whether this is single test method, or two test methods is secondary.
Maybe we should remove that (since it's covered by testInsertForDefaultColumn already).
or, maybe we let testInsertForDefaultColumn test aspects of INSERTS other than NOT NULL constraints?
There was a problem hiding this comment.
I agree. I'll send a PR that applies your comments.
| @Test | ||
| public void testInsertIntoNotNullColumn() | ||
| { | ||
| skipTestUnless(hasBehavior(SUPPORTS_CREATE_TABLE) && hasBehavior(SUPPORTS_INSERT_NOT_NULL_COLUMN)); |
There was a problem hiding this comment.
when !hasBehavior(...), verify that CREATE TABLE t(a .. NOT NULL) is rejected as unsupported (is it?)
| } | ||
|
|
||
| @Test | ||
| @Override |
No description provided.