[ryukobayashi]

What changes were proposed in this pull request?

HiveToSemiJoinRuleSwapInputs is intended to swap inputs only for INNER JOIN plans that can be converted to a semi-join. However, the rule did not check the join type before swapping inputs.

This patch adds an explicit JoinRelType.INNER check before input swapping. It also adds:

• Calcite rule-level regression tests for LEFT JOIN and INNER JOIN
• A qtest reproducing the HIVE-30146 query-result regression

Why are the changes needed?

Without this check, a LEFT JOIN with an aggregate on the left side could have its inputs swapped while retaining the LEFT join type. The subsequent semi-join rule processing could then remove the original left input, changing the preserved-side semantics and producing incorrect results.

For the HIVE-30146 reproduction:

• Before the fix with CBO enabled: COUNT(*) returned 4, and the right-side values were 1, 1, 3, 4
• After the fix: COUNT(*) returns 3, and the right-side values are 1, 1, NULL

Does this PR introduce any user-facing change?

Yes. Queries affected by this optimizer rule now return the correct LEFT JOIN results. There are no SQL syntax, API, or configuration changes.

How was this patch tested?

• Added and ran TestHiveSemiJoinRule
  • Confirmed that LEFT JOIN plans are not rewritten by the swapped-input rules
  • Confirmed that the existing INNER JOIN semi-join conversion still works
• Added and ran hive_30146_left_join.q with TestMiniLlapLocalCliDriver
  • Confirmed that the qtest fails with the expected incorrect results when the fix is reverted
  • Confirmed that the qtest passes after restoring the fix

[deniskuzZ]

+1, pending tests

@ryukobayashi , please check nothing important in sonar report

[konstantinb]

nit: could adding coverage for GROUP BY or UNION (referenced in the story) be beneficial?

[ryukobayashi]

The new testAggregateLeftJoinWithAggregateOnLeftIsNotRewritten covers the Aggregate form: the left input is grouped on the join key, and an Aggregate is placed above the join. UNION is not directly matched by HiveSemiJoinRule, whose operands require a Project or Aggregate directly above the Join. Therefore, I kept this regression test focused on the affected rule shape.

[konstantinb]

nit: perhaps more detailed plan validation similarly to

hive/ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/rules/TestHiveAntiSemiJoinRule.java

Lines 50 to 72 in fc0db4e

	public void testFilterOnNullableColumn() {
	PlanFixture fixture = fixture();
	RelBuilder b = fixture.createRelBuilder();
	// @formatter:off
	RelNode plan = b
	.scan("t1")
	.scan("t2")
	.join(JoinRelType.LEFT, b.equals(
	b.field(2, 0, "t1nullable"),
	b.field(2, 1, "t2id")))
	.filter(b.isNull(b.field("t2nullable")))
	.project(b.field("t1id"))
	.build();
	String expectedPlan = "HiveProject(t1id=[$0])\n"
	+ " HiveFilter(condition=[IS NULL($5)])\n"
	+ " HiveJoin(condition=[=($2, $3)], joinType=[left], algorithm=[none], cost=[not available])\n"
	+ " LogicalTableScan(table=[[t1]])\n"
	+ " LogicalTableScan(table=[[t2]])\n";
	// @formatter:on
	assertPlans(fixture.getPlanner(), plan, expectedPlan, expectedPlan);

would provide more consistent coverage?

[ryukobayashi]

I updated testProjectInnerJoinWithAggregateOnLeftIsRewritten to use assertPlans with explicit pre- and post-optimization plans. The test now validates the complete plan structure, including the join condition and type, the HiveSemiJoin, projections, aggregate, and table scans. This also makes the coverage consistent with TestHiveAntiSemiJoinRule.

[konstantinb]

would it also make sense to add negative tests for RIGHT and FULL joins with an aggregate on the left, grouped on the join key? The ticket says these were never affected because perform() returns early for anything other than INNER or LEFT. The new guard now blocks them one step earlier, in onMatch. Pin both paths so a future change to either can't silently start rewriting these joins.

The test could follow the assertUnchanged pattern used by the LEFT tests, with RIGHT and FULL as the join types.
For a qtest, something like this should return the correct results with CBO on and off (by hand: 4 and 5 rows respectively):

with t1 as (select stack(3, 1, 1, 2) as (k)),
      t2 as (select stack(4, 1, 1, 3, 4) as (k))
 select count(*) from (select distinct k from t1) a right join t2 b on a.k = b.k;

 with t1 as (select stack(3, 1, 1, 2) as (k)),
      t2 as (select stack(4, 1, 1, 3, 4) as (k))
 select count(*) from (select distinct k from t1) a full join t2 b on a.k = b.k;

Note that the aggregate-on-right case (RIGHT/FULL with the aggregate on the right) is still guarded only by perform()'s early return, since the new check is on the swap rule. A test for that orientation would cover it too.

[ryukobayashi]

@konstantinb Thanks for the suggestion. I added regression coverage for RIGHT JOIN and FULL JOIN with an aggregate on the left, covering both the Project and Aggregate forms using assertUnchanged.

I also added coverage for the aggregate-on-right orientation to exercise the existing early return in perform(). The qtest now covers the aggregate-on-left RIGHT JOIN and FULL JOIN cases, with expected row counts of 4 and 5 respectively. The expected output was generated by running the qtest.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[ryukobayashi]

@deniskuzZ The fixes and tests are complete, could you please check it?

[deniskuzZ]

@konstantinb, please review if all your comments were addressed and we could merge