You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/02/07 09:10:30 UTC
[GitHub] [calcite] danny0405 opened a new pull request #1790: [CALCITE-3769]
Deprecate TableScanRule
danny0405 opened a new pull request #1790: [CALCITE-3769] Deprecate TableScanRule
URL: https://github.com/apache/calcite/pull/1790
* Deprecate TableScanRule and always invoke RelOptTable#toRel for
RelBuilder#scan
* RelOptTableImpl#toRel does not translate to EnumerableTableScan
anymore, the EnumerableTableScan conversion logic has been moved to
EnumerableTableScan
* Remove the BindableTableScanRule from the default prepare ruleset, we
only add it where we really need that
* Fix the plan change
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1790:
[CALCITE-3769] Deprecate TableScanRule
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1790: [CALCITE-3769] Deprecate TableScanRule
URL: https://github.com/apache/calcite/pull/1790#discussion_r378197263
##########
File path: core/src/main/java/org/apache/calcite/tools/RelRunners.java
##########
@@ -29,6 +35,19 @@ private RelRunners() {}
/** Runs a relational expression by creating a JDBC connection. */
public static PreparedStatement run(RelNode rel) {
+ final RelShuttle shuttle = new RelHomogeneousShuttle() {
+ @Override public RelNode visit(TableScan scan) {
+ final RelOptTable table = scan.getTable();
+ if (scan instanceof LogicalTableScan
+ && Bindables.BindableTableScan.canHandle(table)) {
+ // Always replace the LogicalTableScan with BindableTableScan
+ // because it's implementation does not require a "schema" as context.
+ return Bindables.BindableTableScan.create(scan.getCluster(), table);
Review comment:
Can the clients of Relrunners do the appropriate conversion instead?
org.apache.calcite.tools.RelRunners#run API says nothing on what changes would it make to the input rel node.
It might be very unexpected for those who see just the call to `RelRunners.run(root))`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 merged pull request #1790: [CALCITE-3769]
Deprecate TableScanRule
Posted by GitBox <gi...@apache.org>.
danny0405 merged pull request #1790: [CALCITE-3769] Deprecate TableScanRule
URL: https://github.com/apache/calcite/pull/1790
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1790:
[CALCITE-3769] Deprecate TableScanRule
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1790: [CALCITE-3769] Deprecate TableScanRule
URL: https://github.com/apache/calcite/pull/1790#discussion_r378245164
##########
File path: core/src/main/java/org/apache/calcite/tools/RelRunners.java
##########
@@ -29,6 +35,19 @@ private RelRunners() {}
/** Runs a relational expression by creating a JDBC connection. */
public static PreparedStatement run(RelNode rel) {
+ final RelShuttle shuttle = new RelHomogeneousShuttle() {
+ @Override public RelNode visit(TableScan scan) {
+ final RelOptTable table = scan.getTable();
+ if (scan instanceof LogicalTableScan
+ && Bindables.BindableTableScan.canHandle(table)) {
+ // Always replace the LogicalTableScan with BindableTableScan
+ // because it's implementation does not require a "schema" as context.
+ return Bindables.BindableTableScan.create(scan.getCluster(), table);
Review comment:
The logic is the same compared to the old one. The RelRunners always run a logical plan.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1790:
[CALCITE-3769] Deprecate TableScanRule
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1790: [CALCITE-3769] Deprecate TableScanRule
URL: https://github.com/apache/calcite/pull/1790#discussion_r378172547
##########
File path: core/src/main/java/org/apache/calcite/tools/RelRunners.java
##########
@@ -29,6 +35,19 @@ private RelRunners() {}
/** Runs a relational expression by creating a JDBC connection. */
public static PreparedStatement run(RelNode rel) {
+ final RelShuttle shuttle = new RelHomogeneousShuttle() {
+ @Override public RelNode visit(TableScan scan) {
+ final RelOptTable table = scan.getTable();
+ if (scan instanceof LogicalTableScan
+ && Bindables.BindableTableScan.canHandle(table)) {
+ // Always replace the LogicalTableScan with BindableTableScan
+ // because it's implementation does not require a "schema" as context.
+ return Bindables.BindableTableScan.create(scan.getCluster(), table);
Review comment:
Because the Enumerables implementation needs a "schema" to look up for the table. but the RelRunners does not actually have a schema, the Bindables actually can implement that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1790:
[CALCITE-3769] Deprecate TableScanRule
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1790: [CALCITE-3769] Deprecate TableScanRule
URL: https://github.com/apache/calcite/pull/1790#discussion_r378080137
##########
File path: core/src/main/java/org/apache/calcite/tools/RelRunners.java
##########
@@ -29,6 +35,19 @@ private RelRunners() {}
/** Runs a relational expression by creating a JDBC connection. */
public static PreparedStatement run(RelNode rel) {
+ final RelShuttle shuttle = new RelHomogeneousShuttle() {
+ @Override public RelNode visit(TableScan scan) {
+ final RelOptTable table = scan.getTable();
+ if (scan instanceof LogicalTableScan
+ && Bindables.BindableTableScan.canHandle(table)) {
+ // Always replace the LogicalTableScan with BindableTableScan
+ // because it's implementation does not require a "schema" as context.
+ return Bindables.BindableTableScan.create(scan.getCluster(), table);
Review comment:
This looks odd.
Can you clarify why is this replacement needed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services