feat: Add HiveTableHandle::columnHandles #15351
Conversation
✅ Deploy Preview for meta-velox canceled.
|
meta-cla
bot
added
the
CLA Signed
mbasmanova
left a comment
mbasmanova
left a comment
There was a problem hiding this comment.
Thanks. A few questions.
velox/connectors/hive/TableHandle.h
Outdated
| return tableParameters_; | ||
| } | ||
|
|
||
| // Full schema including partitioning columns and data columns. |
There was a problem hiding this comment.
Does this need to be full schema or only columns that are used in subfieldFilters and remainingFilter?
There was a problem hiding this comment.
Ideally full schema, but in practice we only look it up for the columns appearing in filters and remaining filter expression
| const RowTypePtr& dataColumns = nullptr, | ||
| const std::unordered_map<std::string, std::string>& tableParameters = {}); | ||
| const std::unordered_map<std::string, std::string>& tableParameters = {}, | ||
| std::vector<HiveColumnHandlePtr> columnHandles = {}); |
There was a problem hiding this comment.
Would you document this new argument?
There was a problem hiding this comment.
I added the document on the accessor method below
| return emptyOutput_; | ||
| } | ||
|
|
||
| void processColumnHandle(const HiveColumnHandlePtr& handle); |
There was a problem hiding this comment.
Would you add a comment to explain what this method is doing? Should we verify consistency between assignments and columnHandles? E.g. what if column x is reported as partition key in assignment and regular column in columnHandles?
There was a problem hiding this comment.
Yeah that's something I have in mind but did not add it, let me add some very strict check for now since it's new
| .startTableScan() | ||
| .outputType(ROW({"x"}, {BIGINT()})) | ||
| .assignments(assignments) | ||
| .dataColumns(asRowType(data[0]->type())) |
There was a problem hiding this comment.
Why do we need both dataColumns and columnHandles?
There was a problem hiding this comment.
dataColumns are needed for schema evolution purpose as well, because column handle does not tell us the position of the column in file. In this test case though we can probably skip it, but in prod usage we always supply it.
There was a problem hiding this comment.
Would be nice to document all pieces of information that go into HiveTableHandle.
There was a problem hiding this comment.
I will beef up the comments on the HiveTableHandle accessors
Summary: Currently if a partitioning column is not projected out, but used in remaining filter, the engine still adds it to assignments in order for the connector to recognize it. This is a hack and we should pass the full column handles inside the table handle. Fix facebookincubator#15348 Reviewed By: mbasmanova Differential Revision: D85979627
Yuhta
force-pushed
the
export-D85979627
branch
from
8703c6b to
0baddb4
Compare
|
This pull request has been merged in 6bb5399. |
3 participants
Summary:
Currently if a partitioning column is not projected out, but used in remaining filter, the engine still adds it to assignments in order for the connector to recognize it. This is a hack and we should pass the full column handles inside the table handle.
Fix #15348
Differential Revision: D85979627