Copilot commented on code in PR #17438:
URL: https://github.com/apache/pinot/pull/17438#discussion_r2653247499
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/request/RequestUtils.java:
##########
@@ -246,7 +248,10 @@ public static Literal getLiteral(SqlLiteral node) {
literal.setNullValue(true);
break;
default:
- literal.setStringValue(StringUtils.replace(node.toValue(), "''",
"'"));
+ boolean useLegacyUnescaping =
+
InstanceConfigProvider.getProperty(CommonConstants.Helix.CONFIG_OF_SSE_LEGACY_LITERAL_UNESCAPING,
+ CommonConstants.Helix.DEFAULT_SSE_LEGACY_LITERAL_UNESCAPING);
+ literal.setStringValue(useLegacyUnescaping ?
StringUtils.replace(node.toValue(), "''", "'") : node.toValue());
Review Comment:
The configuration property is read on every literal conversion, which could
be called many times per query. Consider caching this configuration value as a
static final field initialized during class loading or reading it once per
query to avoid repeated lookups.
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlParserTest.java:
##########
@@ -80,4 +124,166 @@ public static Object[][] nonReservedKeywords() {
new Object[]{"uuid"}
};
}
+
+ // ==================== Tests for SSE legacy literal unescaping behavior
====================
+ //
+ // The legacy behavior performs an additional '' -> ' replacement on strings
that Calcite
+ // has ALREADY unescaped. This test suite verifies:
+ // 1. Simple cases (2 quotes = 1 quote in SQL) work the same with both
settings
+ // 2. Complex cases (4+ quotes) show the double-unescaping problem with
legacy enabled
+
+ @Test
+ public void testSimpleEscapedQuoteBothBehaviorsMatch() {
+ // SQL: 'It''s' represents the string "It's" ('' = escaped single quote)
+ // Calcite unescapes this to "It's"
+ // Both legacy and non-legacy should return "It's" for this simple case
+ String query = "SELECT 'It''s working' FROM testTable";
+
+ // Test with legacy disabled (default)
+ initInstanceConfig(false);
+ PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
+
assertEquals(pinotQuery.getSelectList().get(0).getLiteral().getStringValue(),
"It's working");
+
+ // Test with legacy enabled - same result for simple case
+ initInstanceConfig(true);
+ pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
+
assertEquals(pinotQuery.getSelectList().get(0).getLiteral().getStringValue(),
"It's working");
+ }
+
+ @Test
+ public void testFourQuotesShowsDoubleUnescapingIssue() {
+ // SQL: 'test''''value' - 4 quotes in SQL = 2 escaped quotes = 2 actual
quotes in the string
+ // After Calcite parsing: "test''value" (contains two single quotes)
+ // With legacy (additional '' -> '): "test'value" (WRONG - double
unescaping)
+ // Without legacy: "test''value" (CORRECT)
+ String query = "SELECT 'test''''value' FROM testTable";
+
+ // Without legacy: preserves the two single quotes
+ PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
+
assertEquals(pinotQuery.getSelectList().get(0).getLiteral().getStringValue(),
"test''value");
+
+ initInstanceConfig(false);
+ pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
+
assertEquals(pinotQuery.getSelectList().get(0).getLiteral().getStringValue(),
"test''value");
+
Review Comment:
Lines 162-163 test with the default config (set in @BeforeMethod), then
lines 165-167 explicitly set the same config and repeat the test. This
duplication is unnecessary. Remove lines 162-163 since the explicit test
starting at line 165 is clearer and more intentional.
```suggestion
initInstanceConfig(false);
PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
assertEquals(pinotQuery.getSelectList().get(0).getLiteral().getStringValue(),
"test''value");
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/instance/InstanceConfigProvider.java:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.config.instance;
+
+import com.google.common.annotations.VisibleForTesting;
+import java.util.List;
+import javax.annotation.Nullable;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+
+/**
+ * A singleton class that provides global access to instance configuration.
+ * This can be used by any module to access instance-level configuration that
is set during
+ * instance (controller / broker / server / minion) startup.
+ *
+ * <p>Usage:
+ * <pre>
+ * // During instance startup:
+ * InstanceConfigProvider.setInstanceConfig(pinotConfiguration);
+ *
+ * // From any module:
+ * boolean value =
InstanceConfigProvider.getInstanceConfig().getProperty("config.key",
defaultValue);
+ * </pre>
+ */
+public class InstanceConfigProvider {
+ private static final InstanceConfigProvider INSTANCE = new
InstanceConfigProvider();
+
+ @Nullable
+ private volatile PinotConfiguration _config;
+
+ private InstanceConfigProvider() {
+ }
+
+ public static InstanceConfigProvider getInstance() {
+ return INSTANCE;
+ }
+
+ /**
+ * Returns the underlying PinotConfiguration, or null if not initialized.
+ */
+ @Nullable
+ public static PinotConfiguration getInstanceConfig() {
+ return INSTANCE.getConfig();
+ }
+
+ public static String getProperty(String name, String defaultValue) {
+ if (getInstanceConfig() == null) {
+ return defaultValue;
+ }
+ return getInstanceConfig().getProperty(name, defaultValue);
+ }
+
+ public static boolean getProperty(String name, boolean defaultValue) {
+ if (getInstanceConfig() == null) {
+ return defaultValue;
+ }
+ return getInstanceConfig().getProperty(name, defaultValue);
+ }
+
+ public static int getProperty(String name, int defaultValue) {
+ if (getInstanceConfig() == null) {
+ return defaultValue;
+ }
+ return getInstanceConfig().getProperty(name, defaultValue);
+ }
+
+ public static long getProperty(String name, long defaultValue) {
+ if (getInstanceConfig() == null) {
+ return defaultValue;
+ }
+ return getInstanceConfig().getProperty(name, defaultValue);
+ }
+
+ public static double getProperty(String name, double defaultValue) {
+ if (getInstanceConfig() == null) {
+ return defaultValue;
+ }
+ return getInstanceConfig().getProperty(name, defaultValue);
+ }
+
+ public static List<String> getProperty(String name, List<String>
defaultValue) {
+ if (getInstanceConfig() == null) {
+ return defaultValue;
+ }
+ return getInstanceConfig().getProperty(name, defaultValue);
Review Comment:
The `getProperty` methods call `getInstanceConfig()` twice when the config
is not null, which involves two volatile reads. Store the result in a local
variable to avoid redundant volatile reads and improve performance.
```suggestion
PinotConfiguration instanceConfig = getInstanceConfig();
if (instanceConfig == null) {
return defaultValue;
}
return instanceConfig.getProperty(name, defaultValue);
}
public static boolean getProperty(String name, boolean defaultValue) {
PinotConfiguration instanceConfig = getInstanceConfig();
if (instanceConfig == null) {
return defaultValue;
}
return instanceConfig.getProperty(name, defaultValue);
}
public static int getProperty(String name, int defaultValue) {
PinotConfiguration instanceConfig = getInstanceConfig();
if (instanceConfig == null) {
return defaultValue;
}
return instanceConfig.getProperty(name, defaultValue);
}
public static long getProperty(String name, long defaultValue) {
PinotConfiguration instanceConfig = getInstanceConfig();
if (instanceConfig == null) {
return defaultValue;
}
return instanceConfig.getProperty(name, defaultValue);
}
public static double getProperty(String name, double defaultValue) {
PinotConfiguration instanceConfig = getInstanceConfig();
if (instanceConfig == null) {
return defaultValue;
}
return instanceConfig.getProperty(name, defaultValue);
}
public static List<String> getProperty(String name, List<String>
defaultValue) {
PinotConfiguration instanceConfig = getInstanceConfig();
if (instanceConfig == null) {
return defaultValue;
}
return instanceConfig.getProperty(name, defaultValue);
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -317,6 +317,11 @@ public static class Instance {
// Setting the before serving queries to Integer.MAX_VALUE to effectively
disable throttling by default
public static final String
DEFAULT_MAX_SEGMENT_DOWNLOAD_PARALLELISM_BEFORE_SERVING_QUERIES =
String.valueOf(Integer.MAX_VALUE);
+
+ // SQL parsing
+ public static final String CONFIG_OF_SSE_LEGACY_LITERAL_UNESCAPING =
Review Comment:
The constant name uses the `Instance` prefix (line 322 references 'SSE') but
is defined in the `Instance` class. The naming should clearly indicate its
scope. Consider renaming to `CONFIG_OF_INSTANCE_SSE_LEGACY_LITERAL_UNESCAPING`
or moving it to a more appropriate location if it's specifically for
single-stage engine parsing rather than general instance configuration.
```suggestion
public static final String
CONFIG_OF_INSTANCE_SSE_LEGACY_LITERAL_UNESCAPING =
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
