Skip to content

Commit 4801630

Browse files
committed
refactor(isthmus): make withUnquotedCasing return a copy, not mutate in place
withUnquotedCasing now follows the same copy-returning convention as Calcite's own SqlParser.Config.with*() methods. The field is final and set via the widest constructor (new 8-arg overload). All narrower constructors default to Casing.TO_UPPER as before. This closes the latent footgun where a caller assuming copy semantics would silently mutate shared state, and eliminates the inconsistency between live and snapshotted parser config reads in SqlExpressionToSubstrait.
1 parent 2467c50 commit 4801630

2 files changed

Lines changed: 60 additions & 16 deletions

File tree

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

Lines changed: 53 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,8 @@ public class ConverterProvider {
5555
/** The collection of Substrait extensions (functions and types) available for conversion. */
5656
protected final SimpleExtension.ExtensionCollection extensions;
5757

58-
/**
59-
* The casing applied to unquoted SQL identifiers during parsing. Defaults to {@link
60-
* Casing#TO_UPPER}.
61-
*/
62-
protected Casing unquotedCasing = Casing.TO_UPPER;
58+
/** The casing applied to unquoted SQL identifiers during parsing. */
59+
protected final Casing unquotedCasing;
6360

6461
/** Converter for Substrait scalar functions. */
6562
protected ScalarFunctionConverter scalarFunctionConverter;
@@ -107,7 +104,9 @@ public ConverterProvider(
107104
new ScalarFunctionConverter(extensions.scalarFunctions(), typeFactory),
108105
new AggregateFunctionConverter(extensions.aggregateFunctions(), typeFactory),
109106
new WindowFunctionConverter(extensions.windowFunctions(), typeFactory),
110-
TypeConverter.DEFAULT);
107+
TypeConverter.DEFAULT,
108+
createDefaultExecutionBehavior(),
109+
Casing.TO_UPPER);
111110
}
112111

113112
/**
@@ -127,7 +126,15 @@ public ConverterProvider(
127126
AggregateFunctionConverter afc,
128127
WindowFunctionConverter wfc,
129128
TypeConverter tc) {
130-
this(typeFactory, extensions, sfc, afc, wfc, tc, createDefaultExecutionBehavior());
129+
this(
130+
typeFactory,
131+
extensions,
132+
sfc,
133+
afc,
134+
wfc,
135+
tc,
136+
createDefaultExecutionBehavior(),
137+
Casing.TO_UPPER);
131138
}
132139

133140
/**
@@ -149,13 +156,39 @@ public ConverterProvider(
149156
WindowFunctionConverter wfc,
150157
TypeConverter tc,
151158
Plan.ExecutionBehavior executionBehavior) {
159+
this(typeFactory, extensions, sfc, afc, wfc, tc, executionBehavior, Casing.TO_UPPER);
160+
}
161+
162+
/**
163+
* Creates a ConverterProvider with full customization including execution behavior and unquoted
164+
* identifier casing.
165+
*
166+
* @param typeFactory the Calcite type factory to use
167+
* @param extensions the Substrait extension collection to use
168+
* @param sfc the scalar function converter to use
169+
* @param afc the aggregate function converter to use
170+
* @param wfc the window function converter to use
171+
* @param tc the type converter to use
172+
* @param executionBehavior the execution behavior to use for plans
173+
* @param unquotedCasing the casing to apply to unquoted SQL identifiers during parsing
174+
*/
175+
public ConverterProvider(
176+
RelDataTypeFactory typeFactory,
177+
SimpleExtension.ExtensionCollection extensions,
178+
ScalarFunctionConverter sfc,
179+
AggregateFunctionConverter afc,
180+
WindowFunctionConverter wfc,
181+
TypeConverter tc,
182+
Plan.ExecutionBehavior executionBehavior,
183+
Casing unquotedCasing) {
152184
this.typeFactory = typeFactory;
153185
this.extensions = extensions;
154186
this.scalarFunctionConverter = sfc;
155187
this.aggregateFunctionConverter = afc;
156188
this.windowFunctionConverter = wfc;
157189
this.typeConverter = tc;
158190
this.executionBehavior = executionBehavior;
191+
this.unquotedCasing = unquotedCasing;
159192
}
160193

161194
/**
@@ -181,18 +214,26 @@ public Casing getUnquotedCasing() {
181214
}
182215

183216
/**
184-
* Sets the casing applied to unquoted SQL identifiers during parsing and returns this instance
185-
* for method chaining.
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)}.
186220
*
187221
* <p>The default is {@link Casing#TO_UPPER}. Use {@link Casing#UNCHANGED} to preserve the
188222
* original casing of unquoted identifiers in SQL statements.
189223
*
190224
* @param unquotedCasing the casing to apply to unquoted identifiers
191-
* @return this {@code ConverterProvider} instance
225+
* @return a new {@code ConverterProvider} with the specified casing
192226
*/
193227
public ConverterProvider withUnquotedCasing(Casing unquotedCasing) {
194-
this.unquotedCasing = unquotedCasing;
195-
return this;
228+
return new ConverterProvider(
229+
typeFactory,
230+
extensions,
231+
scalarFunctionConverter,
232+
aggregateFunctionConverter,
233+
windowFunctionConverter,
234+
typeConverter,
235+
executionBehavior,
236+
unquotedCasing);
196237
}
197238

198239
/**

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

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,13 @@ void withUnquotedCasingToLower() {
4141
}
4242

4343
@Test
44-
void withUnquotedCasingIsChainable() {
45-
ConverterProvider provider = new ConverterProvider();
46-
ConverterProvider returned = provider.withUnquotedCasing(Casing.UNCHANGED);
47-
assertEquals(provider, returned);
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());
4851
}
4952

5053
/**

0 commit comments

Comments
 (0)