Avoid re-resolving column names after analysis #25240
Conversation
martint
force-pushed
the
masks
branch
2 times, most recently
from
898190f to
ef453bb
Compare
Record the fields directly and extract the column name from them when needed.
There was a problem hiding this comment.
This definitely looks like a better solution and one small step towards #17, thanks :) In terms of performance overhead, though, it looks like a wash - one linear algorithm gets replaced by some other linear processing paths. But I guess we can't avoid that.
| Set<Field> fields = metadata.getTableSchema(session, tableHandle) | ||
| .columns().stream() | ||
| .map(column -> Field.newQualified( | ||
| node.getTableName(), | ||
| Optional.of(column.getName()), | ||
| column.getType(), | ||
| column.isHidden(), | ||
| Optional.of(tableName), | ||
| Optional.of(column.getName()), | ||
| false)) | ||
| .collect(Collectors.toSet()); |
There was a problem hiding this comment.
In general, that PR is what I started doing, but this is the place where I got into trouble, because I didn't know where to get a Field instance that I would put into the collection of referenced columns.
| .map(Expression::toString))) | ||
| .distinct() |
There was a problem hiding this comment.
Previously .distinct() worked on String, so it made sense to do it early. Now we're processing Field instances, which are not comparable, so .distinct() is moved to a later stage, where we convert them to ColumnInfo, which are comparable.
| columns.stream() | ||
| .map(Field::getOriginColumnName) | ||
| .map(Optional::get) | ||
| .collect(Collectors.toSet())))); |
| @@ -46,4 +47,19 @@ public Optional<String> getMask() | |||
| { | |||
| return mask; | |||
| } | |||
|
|
|||
| @Override | |||
There was a problem hiding this comment.
Should we migrate this class to record?
There was a problem hiding this comment.
This is an SPI class, although marked as @Unstable, so we can migrate it to record, but still we should do it carefully
| Optional.of(tableName), | ||
| Optional.of(column.getName()), | ||
| false)) | ||
| .collect(Collectors.toSet()); |
There was a problem hiding this comment.
immutable set and static import?
There was a problem hiding this comment.
It seems this is a convention is this class, not sure we should be changing that (at this time)
@ksobolew what do you mean? I see |
Now we linearly converts columns to |
|
Yes, but now you are doing this loop only once, and then any lookup in the analysis is constant. |
I guess you're right |
Record the fields directly and extract the column name from them when needed.
Follow up to https://github.com/trinodb/trino/pull/24055/files#r1952735457
cc @kokosing @ksobolew
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.