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() |
| 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?
| Optional.of(tableName), | ||
| Optional.of(column.getName()), | ||
| false)) | ||
| .collect(Collectors.toSet()); |
There was a problem hiding this comment.
immutable set and static import?
@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.