Skip to content

Quote identifier components in toString() #3764

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Quote identifier components in toString() #3764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ...al-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/Identifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.List;
import java.util.Objects;
import java.util.function.Function;
import java.util.stream.Collectors;

@API(API.Status.EXPERIMENTAL)
public class Identifier {
Expand Down Expand Up @@ -59,7 +60,11 @@ public boolean isQualified() {

@Override
public String toString() {
return String.join(".", qualifier) + (qualifier.isEmpty() ? "" : ".") + name;
return qualifier.stream()
.map(q -> "\"" + q + "\"")
.collect(Collectors.joining("."))
+ (qualifier.isEmpty() ? "" : ".")
+ "\"" + name + "\"";
Comment on lines +64 to +67
Copy link
Collaborator

@alecgrieser alecgrieser Nov 18, 2025

Choose a reason for hiding this comment

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

Using double quotes in error messages can lead to problems when integrating with logging frameworks. For example, if the user prints out the error message to a logging system with key-value pairs of quoted keys and values, then the internal quote within the message can end up confusing things. We could get around this by using single quotes (or backticks), though that is different from what the SQL query coming in will do

}

@Nonnull
Expand Down
2 changes: 1 addition & 1 deletion ...e/src/main/java/com/apple/foundationdb/relational/recordlayer/query/SemanticAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ public Expression resolveIdentifier(@Nonnull Identifier identifier,
return resolvedMaybe.get();
}
}
Assert.failUnchecked(ErrorCode.UNDEFINED_COLUMN, String.format(Locale.ROOT, "Attempting to query non existing column '%s'", identifier));
Assert.failUnchecked(ErrorCode.UNDEFINED_COLUMN, String.format(Locale.ROOT, "Attempting to query non existing column %s", identifier));
return null; // unreachable.
}

Expand Down
4 changes: 2 additions & 2 deletions fdb-relational-core/src/test/java/com/apple/foundationdb/relational/api/ddl/IndexTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,7 @@ void failToCreateVersionIndexWithUnknownTable() throws Exception {
"CREATE TABLE T1(col1 bigint, primary key(col1)) " +
"CREATE INDEX mv1 AS SELECT t2.\"__ROW_VERSION\" FROM T1 AS t ORDER BY t2.\"__ROW_VERSION\" " +
"WITH OPTIONS(store_row_versions=true)";
shouldFailWith(stmt, ErrorCode.UNDEFINED_COLUMN, "Attempting to query non existing column 'T2.__ROW_VERSION'");
shouldFailWith(stmt, ErrorCode.UNDEFINED_COLUMN, "Attempting to query non existing column \"T2\".\"__ROW_VERSION\"");
}

@Test
Expand Down Expand Up @@ -680,7 +680,7 @@ void failToCreateVersionIndexWithAmbiguousSource() throws Exception {
"CREATE TABLE T1(col1 bigint, a A Array, primary key(col1)) " +
"CREATE INDEX mv1 AS SELECT X.col2, \"__ROW_VERSION\" FROM T1, (SELECT col2 FROM T1.A) X ORDER BY X.col2, \"__ROW_VERSION\" " +
"WITH OPTIONS(store_row_versions=true)";
shouldFailWith(stmt, ErrorCode.AMBIGUOUS_COLUMN, "Ambiguous reference '__ROW_VERSION'");
shouldFailWith(stmt, ErrorCode.AMBIGUOUS_COLUMN, "Ambiguous reference '\"__ROW_VERSION\"'");
Copy link
Collaborator

@alecgrieser alecgrieser Nov 18, 2025

Choose a reason for hiding this comment

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

Should we update "Ambiguous reference" to remove the single quotes like was done with "Attempting to query non existing column"?

}

@Test
Expand Down
10 changes: 5 additions & 5 deletions .../src/test/java/com/apple/foundationdb/relational/recordlayer/PlanGenerationStackTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public Stream<? extends Arguments> provideArguments(final ExtensionContext conte
Arguments.of(1, "select * from restaurant where rest_no > -10", null),
Arguments.of(2, "sElEct * FrOm rESTaurant", null),
Arguments.of(3, " select * from restaurant", null),
Arguments.of(4, "sElEct * FrOm \"RestaUrantRecord\"", "Unknown table RestaUrantRecord"),
Arguments.of(4, "sElEct * FrOm \"RestaUrantRecord\"", "Unknown table \"RestaUrantRecord\""),
Arguments.of(5, "incorrect ", "incorrect[[]]^^^^^^^^^"),
Arguments.of(6, "select * union restaurant", "select * union restaurant[[]] ^^^^^^^^^^"),
Arguments.of(7, "select * from restaurant where rest_no > 10 ", null),
Expand All @@ -97,7 +97,7 @@ public Stream<? extends Arguments> provideArguments(final ExtensionContext conte
Arguments.of(11, "select * from restaurant where rest_no <= 10 ", null),
Arguments.of(12, "select * from restaurant where rest_no is null ", null),
Arguments.of(13, "select * from restaurant where rest_no is not null ", null),
Arguments.of(14, "select * from restaurant where NON_EXISTING > 10 ", "Attempting to query non existing column 'NON_EXISTING'"),
Arguments.of(14, "select * from restaurant where NON_EXISTING > 10 ", "Attempting to query non existing column \"NON_EXISTING\""),
Arguments.of(15, "select * from restaurant where rest_no > 'hello'", "unable to encapsulate comparison operation due to type mismatch(es)"),
Arguments.of(16, "select * from restaurant where rest_no > 10 AND rest_no < 20", null),
Arguments.of(17, "select * from restaurant where rest_no < 10 AND rest_no < 20", null),
Expand Down Expand Up @@ -149,14 +149,14 @@ public Stream<? extends Arguments> provideArguments(final ExtensionContext conte
Arguments.of(63, "select * from restaurant USE INDEX (record_name_idx), USE INDEX (reviewer_name_idx) where rest_no > 10 ", "Unknown index(es) REVIEWER_NAME_IDX"),
Arguments.of(64, "select * from restaurant with continuation", "syntax error[[]]select * from restaurant with continuation[[]] ^^"),
Arguments.of(65, "select X.rest_no from (select rest_no from restaurant where 42 >= rest_no OR 42 > rest_no) X", null),
Arguments.of( 66, "select X.UNKNOWN from (select rest_no from restaurant where 42 >= rest_no OR 42 > rest_no) X", "Attempting to query non existing column 'X.UNKNOWN'"),
Arguments.of( 66, "select X.UNKNOWN from (select rest_no from restaurant where 42 >= rest_no OR 42 > rest_no) X", "Attempting to query non existing column \"X\".\"UNKNOWN\""),
Arguments.of(67, "select X.rest_no from (select Y.rest_no from (select rest_no from restaurant where 42 >= rest_no OR 42 > rest_no) Y where 42 >= Y.rest_no OR 42 > Y.rest_no) X", null),
Arguments.of(68, "select X.rating from restaurant AS Rec, (select rating from Rec.reviews) X", null),
Arguments.of(69, "select COUNT(MAX(Y.rating)) FROM (select rest_no, X.rating from restaurant AS Rec, (select rating from Rec.reviews) X) as Y GROUP BY Y.rest_no", "unsupported nested aggregate(s) count(max_l"),
Arguments.of(70, "select rating from restaurant GROUP BY rest_no", "Attempting to query non existing column 'RATING'"),
Arguments.of(70, "select rating from restaurant GROUP BY rest_no", "Attempting to query non existing column \"RATING\""),
// TODO understand why the query below cannot be planned
//Arguments.of(71, "select rating + rest_no, MAX(rest_no) from (select rest_no, X.rating from restaurant AS Rec, (select rating from Rec.reviews) X) as Y GROUP BY rest_no, rating", null),
Arguments.of(72, "insert into restaurant_reviewer values (42, \"wrong\", null, null)", "Attempting to query non existing column 'wrong'"),
Arguments.of(72, "insert into restaurant_reviewer values (42, \"wrong\", null, null)", "Attempting to query non existing column \"wrong\""),
Arguments.of(73, "with recursive c as (with c as (select * from restaurant) select * from c) select * from c", "ambiguous nested recursive CTE name"),
Arguments.of(74, "with recursive c as (with c1 as (select * from restaurant) select * from c1) select * from c", null),
Arguments.of(75, "with recursive c as (select * from t, c) select * from c", "recursive CTE does not contain non-recursive term"),
Expand Down
2 changes: 1 addition & 1 deletion ...al-core/src/test/java/com/apple/foundationdb/relational/recordlayer/QueryLoggingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ void testLogException() {
Assert.assertThrows(Exception.class, () -> statement.executeQuery("SELECT * FROM REST where rest_no = 0 OPTIONS (LOG QUERY)"));
final var thrown = logAppender.getLastLogEvent().getThrown();
org.junit.jupiter.api.Assertions.assertNotNull(thrown);
Assertions.assertThat(thrown).hasMessage("Unknown table REST")
Assertions.assertThat(thrown).hasMessage("Unknown table \"REST\"")
.hasNoCause();
}

Expand Down
12 changes: 6 additions & 6 deletions ...t/java/com/apple/foundationdb/relational/recordlayer/query/CaseSensitivityQueryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,14 @@ void caseSensitiveConnectionTest() throws Exception {
try (var statement = connection.createStatement()) {
RelationalAssertions.assertThrowsSqlException(() -> statement.execute("select * from restaurant"))
.hasErrorCode(ErrorCode.UNDEFINED_TABLE)
.hasMessageContaining("Unknown table RESTAURANT");
.hasMessageContaining("Unknown table \"RESTAURANT\"");
}

connection.setOption(Options.Name.CASE_SENSITIVE_IDENTIFIERS, true);
try (var statement = connection.createStatement()) {
RelationalAssertions.assertThrowsSqlException(() -> statement.execute("select * from restaurant"))
.hasErrorCode(ErrorCode.UNDEFINED_TABLE)
.hasMessageContaining("Unknown table restaurant");
.hasMessageContaining("Unknown table \"restaurant\"");
}
}
}
Expand All @@ -96,14 +96,14 @@ void caseSensitiveConnectionTestCase2() throws Exception {
try (var statement = connection.createStatement()) {
RelationalAssertions.assertThrowsSqlException(() -> statement.execute("select * from \"restaurant\""))
.hasErrorCode(ErrorCode.UNDEFINED_TABLE)
.hasMessageContaining("Unknown table restaurant");
.hasMessageContaining("Unknown table \"restaurant\"");
}

connection.setOption(Options.Name.CASE_SENSITIVE_IDENTIFIERS, true);
try (var statement = connection.createStatement()) {
RelationalAssertions.assertThrowsSqlException(() -> statement.execute("select * from ResTaurant"))
.hasErrorCode(ErrorCode.UNDEFINED_TABLE)
.hasMessageContaining("Unknown table ResTaurant");
.hasMessageContaining("Unknown table \"ResTaurant\"");
}
}
}
Expand All @@ -130,14 +130,14 @@ void caseSensitiveConnectionTestCase3(boolean isCaseSensitive) throws Exception
try (var statement = connection.createStatement()) {
RelationalAssertions.assertThrowsSqlException(() -> statement.execute("select * from restaurant"))
.hasErrorCode(ErrorCode.UNDEFINED_TABLE)
.hasMessageContaining("Unknown table RESTAURANT");
.hasMessageContaining("Unknown table \"RESTAURANT\"");
}

connection.setOption(Options.Name.CASE_SENSITIVE_IDENTIFIERS, true);
try (var statement = connection.createStatement()) {
RelationalAssertions.assertThrowsSqlException(() -> statement.execute("select * from restaurant"))
.hasErrorCode(ErrorCode.UNDEFINED_TABLE)
.hasMessageContaining("Unknown table restaurant");
.hasMessageContaining("Unknown table \"restaurant\"");
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions ...test/java/com/apple/foundationdb/relational/recordlayer/query/TemporaryFunctionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1023,7 +1023,7 @@ void attemptToCreateTemporaryFunctionWithDifferentCaseSensitivityOptionCase1() t
RelationalAssertions.assertThrowsSqlException(() -> statement.execute("create temporary function sq1(in x bigint) " +
"on commit drop function as select * from t1 where a < 40 + x "))
.hasErrorCode(ErrorCode.UNDEFINED_TABLE)
.hasMessageContaining("Unknown table T1");
.hasMessageContaining("Unknown table \"T1\"");
}
connection.rollback();
}
Expand All @@ -1045,7 +1045,7 @@ void attemptToCreateTemporaryFunctionWithDifferentCaseSensitivityOptionCase2() t
RelationalAssertions.assertThrowsSqlException(() -> statement.execute("create temporary function sq1(in x bigint) " +
"on commit drop function as select * from t1 where a < 40 + x "))
.hasErrorCode(ErrorCode.UNDEFINED_TABLE)
.hasMessageContaining("Unknown table t1");
.hasMessageContaining("Unknown table \"t1\"");
}
connection.rollback();
}
Expand Down
Loading