vlsi commented on code in PR #2112:
URL: https://github.com/apache/calcite/pull/2112#discussion_r4097648286
##########
core/src/main/java/org/apache/calcite/sql/validate/DelegatingScope.java:
##########
@@ -307,15 +311,13 @@ public SqlQualified fullyQualify(SqlIdentifier
identifier) {
break;
}
// Look for a table alias that is the wrong case.
- if (nameMatcher.isCaseSensitive()) {
+ if (suggestionName == null && nameMatcher.isCaseSensitive()) {
Review Comment:
The `suggestionName == null` condition means the hint comes from the longest
prefix that matches when case is ignored. That rule isn't obvious from the
code, and the next person to edit this loop can easily lose it. Please state it
in a short comment.
This line also changes behavior for three-part identifiers. The loop used to
stop at the first prefix that matched only when case is ignored. Now it
continues, and a shorter prefix that matches exactly wins. Current behavior:
- `select SALES."emp".EXPR$0 from sales.emp, (select ROW(1, 2) as "emp" from
(values 1)) as SALES` used to fail with `Table 'SALES.emp' not found; did you
mean 'EMP'?` and now validates.
- `select SALES."emp".EMPNO from sales.emp, sales.dept as SALES` used to
fail with `Table 'SALES.emp' not found; did you mean 'EMP'?` and now fails with
`Column 'emp' not found in table 'SALES'`.
Please add tests for both, and mention the change in the commit message.
This is the qualified-name case Julian raised in JIRA (`SELECT x.y.z.c` vs
`SELECT z.c`).
##########
core/src/main/java/org/apache/calcite/sql/validate/DelegatingScope.java:
##########
@@ -295,6 +295,10 @@ public SqlQualified fullyQualify(SqlIdentifier identifier)
{
final ResolvedImpl resolved = new ResolvedImpl();
int size = identifier.names.size();
int i = size - 1;
+ // to capture possible suggestion if no table alias found
Review Comment:
The comment doesn't say which suggestion or which alias. Please describe
what the variable holds and when it is used, for example:
```java
// Alias that matches a prefix only when case is ignored; reported as a
// "did you mean" hint if no column matches the identifier either.
```
##########
core/src/main/java/org/apache/calcite/sql/validate/DelegatingScope.java:
##########
@@ -295,6 +295,10 @@ public SqlQualified fullyQualify(SqlIdentifier identifier)
{
final ResolvedImpl resolved = new ResolvedImpl();
int size = identifier.names.size();
int i = size - 1;
+ // to capture possible suggestion if no table alias found
+ SqlIdentifier suggestionPrefix = null;
Review Comment:
These two lines fail the `CheckerFramework (JDK 11)` job on current `main`:
```text
DelegatingScope.java:370: error: [argument.type.incompatible] incompatible
argument for parameter node of newValidationError.
found : @Initialized @Nullable SqlIdentifier
required: @Initialized @NonNull SqlNode
DelegatingScope.java:371: error: [dereference.of.nullable] dereference of
possibly-null reference suggestionPrefix
```
The code sets the two variables together but checks only `suggestionName`
for null, and the checker cannot infer that `suggestionPrefix` is non-null as
well. Please keep a single `@Nullable` variable (the prefix, plus the matched
alias in a small holder or recomputed at the throw site) and annotate it. To
reproduce locally: `./gradlew -PenableCheckerframework :linq4j:classes
:core:classes` on JDK 11.
##########
core/src/main/java/org/apache/calcite/sql/validate/DelegatingScope.java:
##########
@@ -325,6 +327,14 @@ public SqlQualified fullyQualify(SqlIdentifier identifier)
{
final Map<String, ScopeChild> map =
findQualifyingTableNames(columnName, identifier, nameMatcher);
switch (map.size()) {
+ case 0:
+ // Check if found a suggestion earlier
Review Comment:
The grammar is broken, and the comment only restates the `if` below it.
Please delete it, or replace it with the reason the hint is reported here and
not in the loop.
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -10130,6 +10130,12 @@ private static int prec(SqlOperator op) {
checkCustomColumnResolving("T_10");
}
+ @Test void testCustomColumnResolvingAndSimilarAlias() {
Review Comment:
- Please use the file's convention for a regression test: a Javadoc comment
that links the issue, `/** Test case for <a
href="https://issues.apache.org/jira/browse/CALCITE-4025";>[CALCITE-4025] …</a>.
*/`, with the title matching the renamed issue.
- In the comment, state the rule first, then the old defect in one
past-tense sentence: the query used to fail with `Table 'F1' not found; did you
mean 'f1'?`.
- "prevent validator to resolve" should be "prevent the validator from
resolving".
- The name gives the topic but not the outcome. Something like
`testColumnResolvesWhenAliasMatchesOnlyIgnoringCase` would read as a finding in
the test report.
##########
core/src/main/java/org/apache/calcite/sql/validate/DelegatingScope.java:
##########
@@ -325,6 +327,14 @@ public SqlQualified fullyQualify(SqlIdentifier identifier)
{
final Map<String, ScopeChild> map =
findQualifyingTableNames(columnName, identifier, nameMatcher);
switch (map.size()) {
+ case 0:
+ // Check if found a suggestion earlier
+ if (suggestionName != null) {
+ throw validator.newValidationError(suggestionPrefix,
+
RESOURCE.tableNameNotFoundDidYouMean(suggestionPrefix.toString(),
+ suggestionName));
+ }
+ // fall through
default:
Review Comment:
The deferred hint is thrown only from `case 0`. Two other paths still throw
`tableNameNotFound` without it:
- this `default` branch, where the column name matches in two or more tables;
- the `default` branch of `switch (field.getType().getStructKind())` further
down (line 380), where a column matches but is not a PEEK record.
Before this PR, both paths reported the hint. Current behavior:
| Query | Before | After |
| --- | --- | --- |
| `select EMPNO.X from emp as "empno"` | `Table 'EMPNO' not found; did you
mean 'empno'?` | `Table 'EMPNO' not found` |
| `select F1.NOPE from struct.T as "f1"` | `Table 'F1' not found; did you
mean 'f1'?` | `Column 'F1.NOPE' not found in table ''` |
Please route every `tableNameNotFound` in this block through one helper that
checks for the deferred hint, and add a test for each of the two queries above.
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -10130,6 +10130,12 @@ private static int prec(SqlOperator op) {
checkCustomColumnResolving("T_10");
}
+ @Test void testCustomColumnResolvingAndSimilarAlias() {
+ // Check that lower case alias 'f1' doesn't prevent validator to resolve
'F1' column name
+ sql("select F1.C0 from struct.T as \"f1\"")
+ .type("RecordType(INTEGER C0) NOT NULL");
Review Comment:
Continuation lines in this file are indented by 4 spaces, not 6.
This test covers only one outcome of the new logic: the column fallback
finds exactly one PEEK field. The change also affects the other paths, so each
needs its own test:
- the column fallback finds nothing, so the hint is still thrown (name the
existing tests if they already cover it, for example `testBrackets` and
`testLexJava`);
- the column is not a PEEK record, and the column is ambiguous (see the
comment on `DelegatingScope.java` line 338);
- a three-part identifier where a shorter prefix matches an alias exactly
(see the comment on line 314).
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
