Support enforcement of NOT NULL column declarations

[djsagain]

This commit enforces NOT NULL column declarations on write
in the Presto engine, so it applies to all connectors. The
existing Postgres and Mysql tests named testInsertIntoNotNullColumn
were changed to check for the new error message, and a new test
with the same name was added to TestIcebergSmoke.

One possible concern with this commit is that the error message
issued by the Presto engine when writing a null to a NOT NULL
column is a different message than the Connector might issue
if no value was supplied for the NOT NULL column. I think this
is ok, because the error messages supplied by the Connectors are
completely specific to the Connector.

[martint]

plan.getFieldMappings() is guaranteed to match the order of columns in plan.getScope().getRelationType() (i.e., the output "shape" of the query).

The planner is not the right place to match ColumnMetadata to fields, as there is no correspondence between those fields and the order in which the fields appear in the plan.

For instance, given a table t (a BIGINT, b BIGINT), the following query will see the fields in a different order: INSERT INTO t(b, a) VALUES (1, 10)

This should be handled during analysis. The analyzer should record which fields ordinals are supposed to be not null. Take a look at Analysis.JoinUsingAnalysis and callers for an example of how you might record that.

[djsagain]

Ok, I'll look there.

[electrum]

@martint The two callers of this method in this class fetch the columns names from table metadata. Are they wrong as well?

[martint]

For CREATE TABLE AS ... it kind of works out because the metadata is derived from the CREATE TABLE statement. For INSERT INTO, it works because it creates a projection to match the layout of the table metadata. Unfortunately, it relies on projectNode.getOutputSymbols(), which is not guaranteed to come out in the desired order -- it just happens to work today.

[djsagain]

@martint, I believe I've addressed your concern in the latest force push of the commit. The code no longer expects channel order to match column order, and I added a unit test that exercises your INSERT INTO t(b, a) example

[electrum]

Generally looks good. I'll defer to @martint to review the planner changes

[electrum]

We should do this inside the check for not-null channels, since this is logically an optimization of checking the block for nulls. For nullable channels, we don't need to look at this.

[djsagain]

Done in the latest force push.

[electrum]

@martint The two callers of this method in this class fetch the columns names from table metadata. Are they wrong as well?

[martint]

Just a minor comment, but the planner bits look good now.

[martint]

I'd just do this:

Suggested change

	.map(column -> requireNonNull(columnToSymbolMap.get(column.getName()), "columnToSymbolMap is missing column " + column.getName()))
	.collect(Collectors.toSet());
	.map(columnToSymbolMap::get)
	.collect(toImmutableSet());

It's more concise, and the immutable set will catch any nulls that might result from the lookup (which is a bug somewhere in the implementation, anyway)

[djsagain]

Indeed, that's much nicer; changed as you suggested by a forced comment. Thanks very much for looking!

[electrum]

It looks like this caused some tests to start failing with NullPointerException

[martint]

Oh, yeah, sorry, I missed the following in my suggestion above:

.map(ColumnMetadata::getName)

[djsagain]

Doh, I should have looked more closely rather than blindly applying change. I need to be more careful pushing changes at an hour when I'm no longer sentient ;)

It's fixed now in the latest forced push.

[electrum]

Thanks!