Skip to content

Commit ca22330

Browse files
committed
refactor(isthmus): configure unquotedCasing via constructor, not setter
Drop setUnquotedCasing() in favour of constructor parameters, following the same pattern as executionBehavior. This is the only approach that is free from all three issues raised in review: - No naming ambiguity (was withUnquotedCasing, then setUnquotedCasing) - No subclass slicing (copy-return approach silently dropped DynamicConverterProvider overrides) - No mutation window / no need to document call-ordering constraints unquotedCasing is now a final field. Entry points: - new ConverterProvider(Casing) — convenience 1-arg constructor - new ConverterProvider(extensions, typeFactory, Casing) — for custom schemas - existing 8-arg all-args constructor gains Casing as last parameter All narrower constructors default to Casing.TO_UPPER as before. IsthmusEntryPoint switches to new ConverterProvider(unquotedCasing).
1 parent 4801630 commit ca22330

3 files changed

Lines changed: 44 additions & 40 deletions

File tree

isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ enum OutputFormat {
6060
private Casing unquotedCasing = Casing.TO_UPPER;
6161

6262
private ConverterProvider converterProvider() {
63-
return new ConverterProvider().withUnquotedCasing(unquotedCasing);
63+
return new ConverterProvider(unquotedCasing);
6464
}
6565

6666
/**

isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java

Lines changed: 31 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,21 @@ public ConverterProvider() {
8181
this(DefaultExtensionCatalog.DEFAULT_COLLECTION, SubstraitTypeSystem.TYPE_FACTORY);
8282
}
8383

84+
/**
85+
* Creates a ConverterProvider with the specified unquoted identifier casing.
86+
*
87+
* <p>Uses {@link DefaultExtensionCatalog#DEFAULT_COLLECTION} and {@link
88+
* SubstraitTypeSystem#TYPE_FACTORY}.
89+
*
90+
* @param unquotedCasing the casing to apply to unquoted SQL identifiers during parsing
91+
*/
92+
public ConverterProvider(Casing unquotedCasing) {
93+
this(
94+
DefaultExtensionCatalog.DEFAULT_COLLECTION,
95+
SubstraitTypeSystem.TYPE_FACTORY,
96+
unquotedCasing);
97+
}
98+
8499
/**
85100
* Creates a ConverterProvider with the specified extension collection and default type factory.
86101
*
@@ -98,6 +113,21 @@ public ConverterProvider(SimpleExtension.ExtensionCollection extensions) {
98113
*/
99114
public ConverterProvider(
100115
SimpleExtension.ExtensionCollection extensions, RelDataTypeFactory typeFactory) {
116+
this(extensions, typeFactory, Casing.TO_UPPER);
117+
}
118+
119+
/**
120+
* Creates a ConverterProvider with the specified extension collection, type factory, and unquoted
121+
* identifier casing.
122+
*
123+
* @param extensions the Substrait extension collection to use
124+
* @param typeFactory the Calcite type factory to use
125+
* @param unquotedCasing the casing to apply to unquoted SQL identifiers during parsing
126+
*/
127+
public ConverterProvider(
128+
SimpleExtension.ExtensionCollection extensions,
129+
RelDataTypeFactory typeFactory,
130+
Casing unquotedCasing) {
101131
this(
102132
typeFactory,
103133
extensions,
@@ -106,7 +136,7 @@ public ConverterProvider(
106136
new WindowFunctionConverter(extensions.windowFunctions(), typeFactory),
107137
TypeConverter.DEFAULT,
108138
createDefaultExecutionBehavior(),
109-
Casing.TO_UPPER);
139+
unquotedCasing);
110140
}
111141

112142
/**
@@ -213,29 +243,6 @@ public Casing getUnquotedCasing() {
213243
return unquotedCasing;
214244
}
215245

216-
/**
217-
* Returns a new {@code ConverterProvider} identical to this one except with the given unquoted
218-
* identifier casing. Follows the same copy-returning convention as Calcite's own {@code
219-
* SqlParser.Config.withUnquotedCasing(Casing)}.
220-
*
221-
* <p>The default is {@link Casing#TO_UPPER}. Use {@link Casing#UNCHANGED} to preserve the
222-
* original casing of unquoted identifiers in SQL statements.
223-
*
224-
* @param unquotedCasing the casing to apply to unquoted identifiers
225-
* @return a new {@code ConverterProvider} with the specified casing
226-
*/
227-
public ConverterProvider withUnquotedCasing(Casing unquotedCasing) {
228-
return new ConverterProvider(
229-
typeFactory,
230-
extensions,
231-
scalarFunctionConverter,
232-
aggregateFunctionConverter,
233-
windowFunctionConverter,
234-
typeConverter,
235-
executionBehavior,
236-
unquotedCasing);
237-
}
238-
239246
/**
240247
* {@link SqlParser.Config} is a Calcite class which controls SQL parsing behaviour like
241248
* identifier casing.

isthmus/src/test/java/io/substrait/isthmus/UnquotedCasingTest.java

Lines changed: 12 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package io.substrait.isthmus;
22

33
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
import static org.junit.jupiter.api.Assertions.assertSame;
45

56
import io.substrait.isthmus.sql.SubstraitCreateStatementParser;
67
import io.substrait.plan.Plan;
@@ -11,9 +12,9 @@
1112
import org.junit.jupiter.api.Test;
1213

1314
/**
14-
* Verifies that {@link ConverterProvider#withUnquotedCasing(Casing)} controls the unquoted
15-
* identifier casing consistently across both CREATE TABLE parsing and query parsing, so that the
16-
* table name stored in a Substrait {@link NamedScan} reflects the configured casing.
15+
* Verifies that the {@link ConverterProvider} unquoted casing parameter controls identifier casing
16+
* consistently across both CREATE TABLE parsing and query parsing, so that the table name stored in
17+
* a Substrait {@link NamedScan} reflects the configured casing.
1718
*/
1819
class UnquotedCasingTest {
1920

@@ -27,27 +28,23 @@ void defaultCasingIsToUpper() {
2728
}
2829

2930
@Test
30-
void withUnquotedCasingUnchanged() {
31-
ConverterProvider provider = new ConverterProvider().withUnquotedCasing(Casing.UNCHANGED);
31+
void constructorCasingUnchanged() {
32+
ConverterProvider provider = new ConverterProvider(Casing.UNCHANGED);
3233
assertEquals(Casing.UNCHANGED, provider.getSqlParserConfig().unquotedCasing());
3334
assertEquals(Casing.UNCHANGED, provider.getUnquotedCasing());
3435
}
3536

3637
@Test
37-
void withUnquotedCasingToLower() {
38-
ConverterProvider provider = new ConverterProvider().withUnquotedCasing(Casing.TO_LOWER);
38+
void constructorCasingToLower() {
39+
ConverterProvider provider = new ConverterProvider(Casing.TO_LOWER);
3940
assertEquals(Casing.TO_LOWER, provider.getSqlParserConfig().unquotedCasing());
4041
assertEquals(Casing.TO_LOWER, provider.getUnquotedCasing());
4142
}
4243

4344
@Test
44-
void withUnquotedCasingReturnsCopy() {
45-
ConverterProvider original = new ConverterProvider();
46-
ConverterProvider copy = original.withUnquotedCasing(Casing.UNCHANGED);
47-
// returns a distinct instance — original is not mutated
48-
assert copy != original;
49-
assertEquals(Casing.TO_UPPER, original.getUnquotedCasing());
50-
assertEquals(Casing.UNCHANGED, copy.getUnquotedCasing());
45+
void getUnquotedCasingReturnsSameInstance() {
46+
ConverterProvider provider = new ConverterProvider(Casing.UNCHANGED);
47+
assertSame(Casing.UNCHANGED, provider.getUnquotedCasing());
5148
}
5249

5350
/**
@@ -74,7 +71,7 @@ void defaultCasingFoldsTableNameToUpper() throws Exception {
7471
*/
7572
@Test
7673
void unchangedCasingPreservesTableName() throws Exception {
77-
ConverterProvider provider = new ConverterProvider().withUnquotedCasing(Casing.UNCHANGED);
74+
ConverterProvider provider = new ConverterProvider(Casing.UNCHANGED);
7875
Prepare.CatalogReader catalog =
7976
SubstraitCreateStatementParser.processCreateStatementsToCatalog(provider, CREATE_STATEMENT);
8077

0 commit comments

Comments
 (0)