Skip to content

Conversation

@bestbeforetoday
Copy link
Member

@bestbeforetoday bestbeforetoday commented Nov 13, 2025

Since the use of DdlSqlToRelConverter was added to SubstraitSqlToCalcite in commit 260a1c4, the original invocation of SqlToRelConverter that was left as a fallback when it returns null is never called. The reason is that DdlSqlToRelConverter itself already falls back to calling SqlToRelConverter so it never returns null. This means that removeRedundantProjects is never applied to the Rel structure since this only occurs in the uncalled fallback path. All tests continue to pass, which indicates that this optimization is unnecessary.

If it is required in the future, it might be better written as follows to provide flexibility in the rules applied:

static RelRoot applyPlannerRules(RelRoot root, RelOptRule... rules) {
  HepProgramBuilder builder = HepProgram.builder();
  for (RelOptRule rule : rules) {
    builder.addRuleInstance(rule);
  }

  HepPlanner planner = new HepPlanner(builder.build());
  planner.setRoot(root.rel);

  RelNode optimizedExpression = planner.findBestExp();

  return root.withRel(optimizedExpression);
}

Since the use of DdlSqlToRelConverter was added to SubstraitSqlToCalcite
in commit 260a1c4, the original
invocation of SqlToRelConverter that was left as a fallback when it
returns null is never called. The reason is that DdlSqlToRelConverter
itself already falls back to calling SqlToRelConverter so it never
returns null. This means that removeRedundantProjects is never applied
to the Rel structure since this only occurs in the uncalled fallback
path. All tests continue to pass, which indicates that this optimiation
is unnecessary.

If it required in the future, it might be better written as follows to
provide flexibility in the rules applied:

static RelRoot applyPlannerRules(RelRoot root, RelOptRule... rules) {
  HepProgramBuilder builder = HepProgram.builder();
  for (RelOptRule rule : rules) {
    builder.addRuleInstance(rule);
  }

  HepPlanner planner = new HepPlanner(builder.build());
  planner.setRoot(root.rel);

  RelNode optimizedExpression = planner.findBestExp();

  return root.withRel(optimizedExpression);
}

Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
@bestbeforetoday bestbeforetoday marked this pull request as ready for review November 13, 2025 13:30
@andrew-coleman andrew-coleman merged commit 0ec1289 into substrait-io:main Nov 13, 2025
12 checks passed
@bestbeforetoday bestbeforetoday deleted the remove-redundant branch November 13, 2025 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants