[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7873: Fix JSON path expressions in SQL queries.

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on a change in pull request #7873:
URL: https://github.com/apache/pinot/pull/7873#discussion_r767043036



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -1614,40 +1614,48 @@ private static void fixColumnName(String rawTableName, Expression expression, Ma
    * - Case-insensitive cluster
    * - Column name in the format of [table_name].[column_name]
    */
-  private static String getActualColumnName(String rawTableName, String columnName,
+  private static String getActualColumnName(String rawTableName, String sqlColumnName,

Review comment:
       Let's not change the argument name here. If we want to preserve the name for exception message, we can either use a separate variable to store the mutated column name like the current code, or keep it in a separate local variable `String originalColumnName = columnName;`

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -1614,40 +1614,48 @@ private static void fixColumnName(String rawTableName, Expression expression, Ma
    * - Case-insensitive cluster
    * - Column name in the format of [table_name].[column_name]
    */
-  private static String getActualColumnName(String rawTableName, String columnName,
+  private static String getActualColumnName(String rawTableName, String sqlColumnName,
       @Nullable Map<String, String> columnNameMap, @Nullable Map<String, String> aliasMap, boolean isCaseInsensitive) {
-    if ("*".equals(columnName)) {
-      return columnName;
+    if ("*".equals(sqlColumnName)) {
+      return sqlColumnName;
     }
-    // Check if column is in the format of [table_name].[column_name]
-    String[] splits = StringUtils.split(columnName, ".", 2);
-    String columnNameToCheck;
-    if (isCaseInsensitive) {
-      if (splits.length == 2 && rawTableName.equalsIgnoreCase(splits[0])) {
-        columnNameToCheck = splits[1].toLowerCase();
-      } else {
-        columnNameToCheck = columnName.toLowerCase();
-      }
-    } else {
-      if (splits.length == 2 && rawTableName.equals(splits[0])) {
-        columnNameToCheck = splits[1];
-      } else {
-        columnNameToCheck = columnName;
+
+    String columnName = sqlColumnName;
+    // If first part is a table name, then treat the second part as column name; otherwise, treat the entire identifier
+    // as column name.
+    String[] tableSplit = StringUtils.split(columnName, ".", 2);
+    if (tableSplit.length == 2) {
+      if ((isCaseInsensitive && rawTableName.equalsIgnoreCase(tableSplit[0])) || rawTableName.equals(tableSplit[0])) {
+        columnName = tableSplit[1];
       }
     }
+
+    // Split again to check if the column name is suffixed by a path expression.
+    String[] columnSplit = StringUtils.split(columnName, ".", 2);

Review comment:
       This won't work if the column name itself contains `.`. We should first check if the column exist, if not, try to treat it as json path expression.




-- 
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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org