Skip to content

[CALCITE-7757] RelMdFunctionalDependency can infer unsound dependencies and cause incorrect query results - #5235

Open
darpan-14 wants to merge 9 commits into
apache:mainfrom
darpan-14:CALCITE-7757
Open

darpan-14 wants to merge 9 commits into
apache:mainfrom
darpan-14:CALCITE-7757

Conversation

@darpan-14

Copy link
Copy Markdown
Contributor

Jira Link

CALCITE-7757

Changes Proposed

RelMdFunctionalDependency can currently expose unsound or incorrectly indexed dependencies, allowing AggregateRemoveDuplicateKeysRule to remove grouping keys that are still required.

This change:

  • Corrects equality-derived functional dependencies for INNER, LEFT, and RIGHT joins, respecting the null-generating side of outer joins.
  • Preserves valid input functional dependencies through outer joins without incorrectly deriving the reverse dependency from the join equality.
  • Maps aggregate input group ordinals to aggregate output ordinals before publishing functional-dependency metadata.
  • Avoids equality-derived dependencies for FLOAT, REAL, DOUBLE, and INTERVAL types, including nested occurrences.
  • Avoids unsafe expression-derived grouping dependencies for those same types.
  • Adds a reusable SqlTypeUtil predicate traversal for scalar and nested types.
  • Adds regression coverage for each correctness case.

A basic valid duplicate-key removal remains supported:

SELECT t1.col1, t2.col3, COUNT(*)
FROM t1
JOIN t2 ON t1.col1 = t2.col3
GROUP BY t1.col1, t2.col3;

For this inner join, either equivalent equality key may be removed. The new safeguards prevent that reasoning from being applied where null generation, ordinal mapping, or non-reflexive type semantics make it unsound.

Testing

  • ./gradlew :core:test --tests org.apache.calcite.sql.type.SqlTypeUtilTest --tests org.apache.calcite.test.AggregateRemoveDuplicateKeysRuleTest --tests org.apache.calcite.test.RelMetadataTest.testFunctionalDependency*
  • ./gradlew :core:checkstyleMain :core:checkstyleTest
  • git diff --check upstream/main...HEAD

The focused test run completed 49 tests with 0 failures.

@mihaibudiu

Copy link
Copy Markdown
Contributor

CI doesn't like your changes

@darpan-14

Copy link
Copy Markdown
Contributor Author

CI is happy now. 😄

}

/**
* Copies input dependencies into a join, optionally filtering dependencies

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this javadoc. Can you please say what each parameter is and what the function actually computes? There's no "join" to be seen of here.


/**
* Returns whether a dependency's determinant contains a non-nullable input
* field. Such a determinant cannot collide with the all-NULL determinant of

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this comment mention a LEFT JOIN? There is no left join here. Please make the comment describe this function does.

* @param fdSet Functional dependency set
* @param offset Index offset
* @return Shifted functional dependency set
* Adds functional dependencies implied by a join condition.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does it add the dependencies?

* Extracts functional dependencies from equality and AND conditions.
* Handles col1 = col2, col1 IS NOT DISTINCT FROM col2, and AND conditions.
* Adds bidirectional dependencies for input-reference equalities in a
* condition. Callers are responsible for ensuring that every output row

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every output row of what?

@darpan-14

Copy link
Copy Markdown
Contributor Author

Updated all Javadocs raised in review. The helper methods now document their general behavior, parameters, return value where applicable, where inferred dependencies are recorded, and the relational-output requirement for equality-derived dependencies.

@sonarqubecloud

Copy link
Copy Markdown

@Test void testKeepsDerivedGroupKeyForNestedDouble() {
// Determinism alone is insufficient for approximate values, including
// nested occurrences. Keep the derived key unless grouping equality is
// known to be congruent with the expression for ARRAY<DOUBLE>.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot read this comment. What is "congruent with the expression"?

ImmutableBitSet aggCols =
ImmutableBitSet.range(rel.getGroupCount(), rel.getRowType().getFieldCount());
fdBuilder.addArrow(groupSet, aggCols);
fdBuilder.addArrow(ImmutableBitSet.range(rel.getGroupCount()), aggCols);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you also need to add to line 373 above a check Aggregate.isSimple(rel)
Some aggregates like ROLLUP are not simple; maybe you can add a suitable test case:
select a, count(*) c from t group by rollup(a)

@mihaibudiu

Copy link
Copy Markdown
Contributor

When this is ready please re-request a review

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