Respond to Julian feedback

core/src/main/java/org/apache/calcite/sql/SqlDialect.java

@@ -31,10 +31,12 @@
 import org.apache.calcite.rex.RexNode;
 import org.apache.calcite.sql.dialect.JethroDataSqlDialect;
 import org.apache.calcite.sql.fun.SqlInternalOperators;
+import org.apache.calcite.sql.fun.SqlLibraryOperators;
 import org.apache.calcite.sql.fun.SqlStdOperatorTable;
 import org.apache.calcite.sql.parser.SqlParser;
 import org.apache.calcite.sql.parser.SqlParserPos;
 import org.apache.calcite.sql.type.AbstractSqlType;
+import org.apache.calcite.sql.type.SqlTypeName;
 import org.apache.calcite.sql.type.SqlTypeUtil;
 import org.apache.calcite.sql.validate.SqlConformance;
 import org.apache.calcite.sql.validate.SqlConformanceEnum;
@@ -864,21 +866,43 @@ public boolean supportsDataType(RelDataType type) {
     return SqlTypeUtil.convertTypeToSpec(type);
   }
 
-  /** Rewrite SINGLE_VALUE into expression based on database variants
+  /** Rewrites SINGLE_VALUE into expression based on database variants
    * E.g. HSQLDB, MYSQL, ORACLE, etc.
    */
   public SqlNode rewriteSingleValueExpr(SqlNode aggCall, RelDataType relDataType) {
     LOGGER.debug("SINGLE_VALUE rewrite not supported for {}", databaseProduct);
     return aggCall;
   }
 
-  /** With x as BOOLEAN column, rewrite MAX(x)/MIN(x) as BOOL_OR(x)/BOOL_AND(x)
-   * for certain database variants (Postgres and Redshift, currently).
+  /**
+   * Rewrites MAX(x)/MIN(x) as BOOL_OR(x)/BOOL_AND(x) for certain
+   * database variants (Postgres and Redshift, currently).
+   * @see #rewriteMaxMin(SqlNode, RelDataType)
    */
   public SqlNode rewriteMaxMinExpr(SqlNode aggCall, RelDataType relDataType) {
     return aggCall;
   }
 
+  /**
+   * Helper for rewrites of MAX/MIN.
+   * Some dialects (e.g. Postgres and Redshift), rewrite as
+   * BOOL_OR/BOOL_AND if the return type is BOOLEAN.
+   */
+  protected static SqlNode rewriteMaxMin(SqlNode aggCall, RelDataType relDataType) {
+    // The behavior of this method depends on the argument type,
+    // and whether it is MIN/MAX
+    final SqlTypeName type = relDataType.getSqlTypeName();
+    final boolean isMax = aggCall.getKind() == SqlKind.MAX;
+    // If the type is BOOLEAN, create a new call to the correct operator
+    if (type == SqlTypeName.BOOLEAN) {
+      final SqlOperator op = isMax ? SqlLibraryOperators.BOOL_OR : SqlLibraryOperators.BOOL_AND;
+      final SqlNode operand = ((SqlBasicCall) aggCall).operand(0);
+      return op.createCall(SqlParserPos.ZERO, operand);
+    }
+    // Otherwise, just return as it arrived
+    return aggCall;
+  }
+
   /**
    * Returns the SqlNode for emulating the null direction for the given field
    * or <code>null</code> if no emulation needs to be done.

core/src/main/java/org/apache/calcite/sql/dialect/PostgresqlSqlDialect.java

@@ -179,8 +179,7 @@ public PostgresqlSqlDialect(Context context) {
   }
 
   @Override public SqlNode rewriteMaxMinExpr(SqlNode aggCall, RelDataType relDataType) {
-    RedshiftSqlDialect redshiftSqlDialect = new RedshiftSqlDialect(DEFAULT_CONTEXT);
-    return redshiftSqlDialect.rewriteMaxMinExpr(aggCall, relDataType);
+    return rewriteMaxMin(aggCall, relDataType);
   }
 
   @Override public boolean supportsGroupByLiteral() {

core/src/main/java/org/apache/calcite/sql/dialect/RedshiftSqlDialect.java

@@ -20,15 +20,11 @@
 import org.apache.calcite.rel.type.RelDataType;
 import org.apache.calcite.rel.type.RelDataTypeSystem;
 import org.apache.calcite.rel.type.RelDataTypeSystemImpl;
-import org.apache.calcite.sql.SqlBasicCall;
 import org.apache.calcite.sql.SqlDataTypeSpec;
 import org.apache.calcite.sql.SqlDialect;
-import org.apache.calcite.sql.SqlKind;
 import org.apache.calcite.sql.SqlNode;
-import org.apache.calcite.sql.SqlOperator;
 import org.apache.calcite.sql.SqlUserDefinedTypeNameSpec;
 import org.apache.calcite.sql.SqlWriter;
-import org.apache.calcite.sql.fun.SqlLibraryOperators;
 import org.apache.calcite.sql.parser.SqlParserPos;
 import org.apache.calcite.sql.type.SqlTypeName;
 
@@ -112,18 +108,7 @@ public RedshiftSqlDialect(Context context) {
   }
 
   @Override public SqlNode rewriteMaxMinExpr(SqlNode aggCall, RelDataType relDataType) {
-    // The behavior of this method depends on the argument type,
-    // and whether it is MIN/MAX
-    final SqlTypeName type = relDataType.getSqlTypeName();
-    final boolean isMax = aggCall.getKind() == SqlKind.MAX;
-    // If the type is BOOLEAN, create a new call to the correct operator
-    if (type == SqlTypeName.BOOLEAN) {
-      final SqlOperator op = isMax ? SqlLibraryOperators.BOOL_OR : SqlLibraryOperators.BOOL_AND;
-      final SqlNode operand = ((SqlBasicCall) aggCall).operand(0);
-      return op.createCall(SqlParserPos.ZERO, operand);
-    }
-    // Otherwise, just return as it arrived
-    return aggCall;
+    return rewriteMaxMin(aggCall, relDataType);
   }
 
   @Override public boolean supportsGroupByLiteral() {

core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java

@@ -6677,9 +6677,9 @@ private void checkLiteral2(String expression, String expected) {
    * Add BITAND_AGG, BITOR_AGG functions (enabled in Snowflake library)</a>. */
   @Test void testBitOrAgg() {
     final String query = "select bit_or(\"product_id\")\n"
-      	+ "from \"product\"";
+        + "from \"product\"";
     final String expectedSnowflake = "SELECT BITOR_AGG(\"product_id\")\n"
-      	+ "FROM \"foodmart\".\"product\"";
+        + "FROM \"foodmart\".\"product\"";
     sql(query).withLibrary(SqlLibrary.SNOWFLAKE).withSnowflake().ok(expectedSnowflake);
   }
 
@@ -6695,6 +6695,10 @@ private void checkLiteral2(String expression, String expected) {
         + "MIN(\"brand_name\" = 'a'), "
         + "MIN(\"brand_name\")\n"
         + "FROM \"foodmart\".\"product\"";
+    final String expectedBigQuery = "SELECT MAX(brand_name = 'a'), "
+        + "MIN(brand_name = 'a'), "
+        + "MIN(brand_name)\n"
+        + "FROM foodmart.product";
     final String expectedPostgres = "SELECT BOOL_OR(\"brand_name\" = 'a'), "
         + "BOOL_AND(\"brand_name\" = 'a'), "
         + "MIN(\"brand_name\")\n"
@@ -6703,9 +6707,11 @@ private void checkLiteral2(String expression, String expected) {
         + "BOOL_AND(\"brand_name\" = 'a'), "
         + "MIN(\"brand_name\")\n"
         + "FROM \"foodmart\".\"product\"";
-    sql(query).ok(expected);
-    sql(query).withPostgresql().ok(expectedPostgres);
-    sql(query).withRedshift().ok(expectedRedshift);
+    sql(query)
+      .ok(expected)
+      .withBigQuery().ok(expectedBigQuery)
+      .withPostgresql().ok(expectedPostgres)
+      .withRedshift().ok(expectedPostgres);
   }
 
   /** Test case for