[GitHub] [pinot] walterddr commented on a diff in pull request #9818: [multistage] m0ar tests! (binary, boolean, char, time)

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

walterddr commented on code in PR #9818:
URL: https://github.com/apache/pinot/pull/9818#discussion_r1025756060


##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/QueryRunnerTestBase.java:
##########
@@ -268,9 +274,18 @@ private static List<String> toH2FieldNamesAndTypes(org.apache.pinot.spi.data.Sch
         case DOUBLE:
           fieldType = "double";
           break;
+        case BOOLEAN:
+          fieldType = "BOOLEAN";
+          break;
         case BIG_DECIMAL:
           fieldType = "NUMERIC";
           break;
+        case BYTES:
+          fieldType = "BYTEA";

Review Comment:
   typo? should this be `"BYTES"`?



##########
pinot-query-runtime/src/test/resources/queries/BooleanLogic.json:
##########
@@ -0,0 +1,106 @@
+{
+  "boolean_type": {
+    "tables": {
+      "bools": {
+        "schema": [{"name": "b", "type": "BOOLEAN"}],
+        "inputs": [[true], [false]]
+      }
+    },
+    "queries": [
+      {
+        "description": "should support booleans",
+        "sql": "select b FROM {bools}"
+      },
+      {
+        "description": "should support boolean literals",
+        "sql": "select b = true, b = false FROM {bools}"
+      }
+    ]
+  },
+  "boolean_implicit_casting": {
+    "tables": {
+      "tbl": {
+        "schema": [
+          {"name": "b", "type": "BOOLEAN"},
+          {"name": "i", "type": "INT"},
+          {"name": "d", "type": "DOUBLE"},
+          {"name": "s", "type": "STRING"},
+          {"name": "n", "type": "BIG_DECIMAL"},
+          {"name": "t", "type": "TIMESTAMP"},
+          {"name": "by", "type": "BYTES"}
+        ],
+        "inputs": [
+          [true, 1, 1.2, "foo", "1.234", "2022-01-02 03:45:00", "DEADBEEF"],
+          [true, 0, 0.0, "", "0.0", "2022-01-02 03:45:00", "00"],
+          [false, 1, 1.2, "foo", "1.234", "2022-01-02 03:45:00", "DEADBEEF"],
+          [false, 0, 0.0, "", "0.0", "2022-01-02 03:45:00", "00"]
+        ]
+      }
+    },
+    "queries": [
+      {
+        "note": "to be postgres compatible, this should throw an exception - but various other systems (like H2 and MySQL) support this",
+        "description": "check implicit cast between boolean and int",
+        "sql": "select b = i FROM {tbl}"

Review Comment:
   can we add some function mix?
   - boolean logic cascade: `(b = i) = true` 
   - boolean logic combine `(b = i) AND/OR (b = n)`



##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -129,6 +130,9 @@ public QueryPlan planQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions)
       plannerContext.setOptions(sqlNodeAndOptions.getOptions());
       RelRoot relRoot = compileQuery(sqlNodeAndOptions.getSqlNode(), plannerContext);
       return toDispatchablePlan(relRoot, plannerContext);
+    } catch (CalciteContextException e) {
+      throw new RuntimeException("Error composing query plan for '" + sqlQuery
+          + "': " + e.getMessage() + "'", e);

Review Comment:
   could we add a better message? what's the benefit for adding this extra catch clause?



##########
pinot-query-planner/src/main/java/org/apache/pinot/query/catalog/PinotCatalog.java:
##########
@@ -63,7 +63,11 @@ public PinotCatalog(TableCache tableCache) {
   @Override
   public Table getTable(String name) {
     String tableName = TableNameBuilder.extractRawTableName(name);
-    return new PinotTable(_tableCache.getSchema(tableName));
+    org.apache.pinot.spi.data.Schema schema = _tableCache.getSchema(tableName);
+    if (schema == null) {
+      throw new IllegalArgumentException("Could not find schema for table: " + tableName);
+    }

Review Comment:
   nit: by the time it reaches here. it shouldn't be couldn't find the schema for a table but more of an internal error as PinotCatalog is inconsistent of the table it said it contains vs. the schema it can pull out from. 
   
   but I don't know what's a better error message to indicate this info. 



-- 
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