Replace tableScan API with tableScanBuilder #7769
Conversation
✅ Deploy Preview for meta-velox canceled.
|
facebook-github-bot
added
the
CLA Signed
|
@mbasmanova any thoughts on this change? Thanks. |
There was a problem hiding this comment.
@majetideepak Overall looks good. The new code is longer, but maybe it is easier to read. I have some questions.
velox/exec/tests/HashJoinTest.cpp
Outdated
| auto op = PlanBuilder(planNodeIdGenerator, pool_.get()) | ||
| .startTableScan() | ||
| .outputType(scanOutputType) | ||
| .tableHandle(makeTableHandle( |
There was a problem hiding this comment.
this can be simplified to makeTableHandle(), no?
velox/exec/tests/TableScanTest.cpp
Outdated
| assignments) | ||
| .startTableScan() | ||
| .outputType(rowType) | ||
| .tableHandle(makeTableHandle( |
There was a problem hiding this comment.
I wonder if this could be just
.remainingFilter("e.a is null")
velox/exec/tests/TableScanTest.cpp
Outdated
| .outputType(ROW({{"d", BIGINT()}})) | ||
| .tableHandle(makeTableHandle( | ||
| SubfieldFilters{}, | ||
| parseExpr("e.a is null or e.b is null", rowType))) |
velox/exec/tests/TableScanTest.cpp
Outdated
| .startTableScan() | ||
| .outputType( | ||
| outputColumn == kNoOutput ? ROW({"d"}, {BIGINT()}) : rowType) | ||
| .tableHandle(makeTableHandle(SubfieldFilters{}, remainingFilter)) |
velox/exec/tests/TableScanTest.cpp
Outdated
| .tableScan(rowType, makeTableHandle(), assignments) | ||
| .startTableScan() | ||
| .outputType(rowType) | ||
| .tableHandle(makeTableHandle()) |
velox/exec/tests/TableScanTest.cpp
Outdated
| .tableScan(ROW({}, {}), {"c1 <= 4000.1"}, "", dataColumns) | ||
| .startTableScan() | ||
| .outputType(ROW({}, {})) | ||
| .subfieldFilters({"c1 <= 4000.1"}) |
There was a problem hiding this comment.
I wonder if it makes sense to allow specifying individual filters separately:
.subfieldFilters("a < 5")
.subfieldFilters("b is null")
velox/exec/tests/TableScanTest.cpp
Outdated
| @@ -1725,7 +1828,7 @@ TEST_F(TableScanTest, statsBasedSkippingNulls) { | |||
|
|
|||
| auto assertQuery = [&](const std::string& filter) { | |||
| return TableScanTest::assertQuery( | |||
| PlanBuilder().tableScan(rowType, {filter}).planNode(), | |||
| PlanBuilder(pool_.get()).tableScan(rowType, {filter}).planNode(), | |||
There was a problem hiding this comment.
Why do we need to pass pool here?
velox/exec/tests/TableScanTest.cpp
Outdated
| @@ -2477,7 +2625,7 @@ TEST_F(TableScanTest, remainingFilterSkippedStrides) { | |||
| } | |||
| createDuckDbTable(vectors); | |||
| core::PlanNodeId tableScanNodeId; | |||
| auto plan = PlanBuilder() | |||
| auto plan = PlanBuilder(pool_.get()) | |||
There was a problem hiding this comment.
Why do we need to pass pool here?
majetideepak
force-pushed
the
tablescanpart2
branch
from
70c9b7d to
0d51fb4
Compare
majetideepak
force-pushed
the
tablescanpart2
branch
from
0d51fb4 to
8451d92
Compare
|
@mbasmanova I addressed your comments. Can you make another pass? I removed most of the redundant |
|
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mbasmanova merged this pull request in fc0538d. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Summary: Remove the tableScan API that is less popular to keep the planBuilder API clean. Pull Request resolved: facebookincubator#7769 Reviewed By: xiaoxmeng Differential Revision: D51971797 Pulled By: mbasmanova fbshipit-source-id: 3f63f937a9371ec176213260d369d87d49ba1c51
Remove the tableScan API that is less popular to keep the planBuilder API clean.