Skip to content

Conversation

@lincoln-lil
Copy link
Contributor

@lincoln-lil lincoln-lil commented Feb 5, 2025

What is the purpose of the change

This is a trivial fix for string comparison in FlinkCalcMergeRule.
However it doesn't actually affect the optimization results, so no new cases were added to the fix.
But we can verify this detail by debugging FlinkCalcMergeRule's optimization process with the following case(add to FlinkCalcMergeRuleTest):

@Test
def testCalcMergeWithTrivialCalc(): Unit = {
  val testTable = "MyTable"
  val relBuilder = util.getPlanner.plannerContext.createRelBuilder()
  val flinkLogicalTraits = relBuilder.getCluster.traitSetOf(FlinkConventions.LOGICAL)
  val table = relBuilder.getRelOptSchema
    .asInstanceOf[CalciteCatalogReader]
    .getTable(ImmutableList.of(testTable))
    .asInstanceOf[FlinkPreparingTableBase]
  val flinkLogicalTableScan = new FlinkLogicalDataStreamTableScan(
    relBuilder.getCluster,
    flinkLogicalTraits,
    Collections.emptyList[RelHint](),
    table)

  relBuilder.scan(testTable)
  val logicScan = relBuilder.peek(0)
  val rowType = logicScan.getRowType
  val projects = (0 until (rowType.getFieldCount)).map(f => relBuilder.field(f)).toList
  val program = RexProgram.create(
    rowType,
    projects,
    null,
    rowType,
    relBuilder.getRexBuilder
  )

  val bottomCalc = FlinkLogicalCalc.create(flinkLogicalTableScan, program)
  // create a trivial calc
  val topCalc = FlinkLogicalCalc.create(bottomCalc, program)

  val optimizedRels = util.getPlanner.optimize(Array(topCalc))
  print(bottomCalc.getRelDigest, optimizedRels.get(0).getRelDigest)
  assertThat(bottomCalc.getRelDigest).isEqualTo(optimizedRels.get(0).getRelDigest)
  //    util.assertPlanEquals(Array(topCalc), Array.empty[ExplainDetail], withRowType = false, Array(PlanKind.OPT_REL), () => Unit)
}

Brief change log

fix string comparison

Verifying this change

existing tests

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @public(Evolving): (no)
  • The serializers: (no )
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)

@flinkbot
Copy link
Collaborator

flinkbot commented Feb 5, 2025

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@lincoln-lil
Copy link
Contributor Author

@snuyanzin Could you help look at this minor fix?

@snuyanzin
Copy link
Contributor

should we have a backport to 2.0?

@lincoln-lil
Copy link
Contributor Author

should we have a backport to 2.0?

I think so, will create a backport pr soon.

@lincoln-lil lincoln-lil merged commit c3c7f0c into apache:master Feb 5, 2025
@lincoln-lil lincoln-lil deleted the hotfix-calcmergerule branch February 5, 2025 13:48
lincoln-lil added a commit to lincoln-lil/flink that referenced this pull request Feb 5, 2025
@lincoln-lil
Copy link
Contributor Author

@snuyanzin Thanks for reviewing this! Backport pr #26110 to 2.0 branch has been created.

ryanvanhuuksloot pushed a commit to ryanvanhuuksloot/flink that referenced this pull request Feb 19, 2025
Au-Miner pushed a commit to Au-Miner/flink that referenced this pull request Feb 23, 2025
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.

3 participants