Skip to content

Make UPDATE respect non nullability of columns#13644

Merged
martint merged 1 commit intotrinodb:masterfrom
homar:homar/fix_non_nullability_in_update
Aug 19, 2022
Merged

Make UPDATE respect non nullability of columns#13644
martint merged 1 commit intotrinodb:masterfrom
homar:homar/fix_non_nullability_in_update

Conversation

@homar
Copy link
Copy Markdown
Member

@homar homar commented Aug 12, 2022

Description

Is this change a fix, improvement, new feature, refactoring, or other?

a fix

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

core engine

How would you describe this change to a non-technical end user or system administrator?

it makes update queries respect non nullable columns and not insert nulls into them

Related issues, pull requests, and links

Fixes: #13435

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Core
* Makes Update queries respect non nullability of columns. ({https://github.com/trinodb/trino/issues/13435}`13435`)

@cla-bot cla-bot bot added the cla-signed label Aug 12, 2022
@homar homar force-pushed the homar/fix_non_nullability_in_update branch 2 times, most recently from e43a179 to 064f753 Compare August 13, 2022 00:04
@homar
Copy link
Copy Markdown
Member Author

homar commented Aug 13, 2022

failure is the same as here #13658

@homar homar marked this pull request as ready for review August 13, 2022 12:31
@homar homar changed the title [WIP] Make update respect non nullability of columns Make update respect non nullability of columns Aug 13, 2022
@homar
Copy link
Copy Markdown
Member Author

homar commented Aug 16, 2022

TestDeltaLakeDatabricksInsertCompatibility.testCompressionWithOptimizedWriter seems to be a transient issue

And singlestore failed because of #13677 (review) which should be fixed already

ImmutableList.Builder<Symbol> updatedColumnValuesBuilder = ImmutableList.builder();
orderedColumnValues.forEach(columnValue -> updatedColumnValuesBuilder.add(planAndMappings.get(columnValue)));
TableMetadata tableMetadata = metadata.getTableMetadata(session, handle);
ImmutableMap<String, Integer> updatedColumnNameToIndex = IntStream.range(0, updatedColumnNames.size())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declare as Map

TableMetadata tableMetadata = metadata.getTableMetadata(session, handle);
ImmutableMap<String, Integer> updatedColumnNameToIndex = IntStream.range(0, updatedColumnNames.size())
.boxed()
.collect(toImmutableMap(updatedColumnNames::get, i -> i));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i -> i -> index -> index. Or static import on Function.identity()

ImmutableMap<String, Integer> updatedColumnNameToIndex = IntStream.range(0, updatedColumnNames.size())
.boxed()
.collect(toImmutableMap(updatedColumnNames::get, i -> i));
ImmutableList.Builder<Symbol> updatedNonNullableColumnsValuesBuilder = ImmutableList.builder();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep old variable name: updatedColumnValuesBuilder

Assignments.Builder assignmentsBuilder = Assignments.builder();
assignmentsBuilder.putIdentities(builder.getRoot().getOutputSymbols());

for (ColumnMetadata column : tableMetadata.getColumns()) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels the iteration would be simpler if you just iterate in parallel over orderedColumValues and targetColumnNames. IIUC those have same length so you can just do the iteration using integer index from 0..len(orderedColumValues).
The you would just look at columns you are interested in and look up for extra information (about nullability) by looking into tableMetadata. Currently you are iterating over all the columns and skipping once you are not interested in. This is counterintutive and requires using updatedColumnNameToIndex.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So actually i thought about that but how would you look into tableMetadata for specific column ?
There is getColumn method but its implementation doesn't seem very efficient:

public ColumnMetadata getColumn(String name)
    {
        return getColumns().stream()
                .filter(columnMetadata -> columnMetadata.getName().equals(name))
                .findFirst()
                .orElseThrow(() -> new IllegalArgumentException("Invalid column name: " + name));
    }

We may end up going over that column list many many times. WDYT @losipiuk

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just cache it as a name->CM map?

@homar homar force-pushed the homar/fix_non_nullability_in_update branch 2 times, most recently from 2f9c5ae to 64080ed Compare August 18, 2022 20:33
@homar homar force-pushed the homar/fix_non_nullability_in_update branch 2 times, most recently from 5e3e35c to 42a4caf Compare August 18, 2022 22:30
@homar homar force-pushed the homar/fix_non_nullability_in_update branch from 42a4caf to 76a761b Compare August 18, 2022 22:42
@homar homar force-pushed the homar/fix_non_nullability_in_update branch from 76a761b to 1e5b906 Compare August 19, 2022 07:56
@homar homar force-pushed the homar/fix_non_nullability_in_update branch from 1e5b906 to 9f3ef15 Compare August 19, 2022 18:21
@homar
Copy link
Copy Markdown
Member Author

homar commented Aug 19, 2022

@martint can we merge it ? or do you still have some concerns ?

@martint martint merged commit 2be6e44 into trinodb:master Aug 19, 2022
@github-actions github-actions bot added this to the 394 milestone Aug 19, 2022
@findepi findepi changed the title Make update respect non nullability of columns Make UPDATE respect non nullability of columns Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Updates don't respect columns non nullability

3 participants