matriv commented on a change in pull request #18858:
URL: https://github.com/apache/flink/pull/18858#discussion_r810930664
##########
File path:
flink-table/flink-table-api-java/src/test/java/org/apache/flink/table/utils/CatalogManagerMocks.java
##########
@@ -33,7 +35,15 @@
public static final String DEFAULT_DATABASE =
EnvironmentSettings.DEFAULT_BUILTIN_DATABASE;
public static CatalogManager createEmptyCatalogManager() {
- final CatalogManager catalogManager = preparedCatalogManager().build();
+ return createEmptyCatalogManager(null);
+ }
+
+ public static CatalogManager createEmptyCatalogManager(@Nullable Catalog
catalog) {
+ final CatalogManager.Builder builder = preparedCatalogManager();
+ if (catalog != null) {
Review comment:
If it's null we will have an NPE later, should we already throw a more
meaningful exception?
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/utils/JavaUserDefinedScalarFunctions.java
##########
@@ -101,7 +101,7 @@ public void close() {
/** Testing open method is called. */
public static class UdfWithOpen extends ScalarFunction {
- private boolean isOpened = false;
+ private transient boolean isOpened = false;
Review comment:
Could you add a comment why it's needed?
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/typeutils/SymbolUtil.java
##########
@@ -556,8 +595,10 @@ private static void addSymbolMapping(
}
private static void checkCalciteSymbol(Enum<?> calciteSymbol) {
+ final String className = calciteSymbol.getClass().getName();
checkArgument(
-
calciteSymbol.getClass().getName().startsWith("org.apache.calcite."),
+ className.startsWith("org.apache.calcite.")
+ || className.startsWith("com.google.common.collect."),
Review comment:
why is that needed?
##########
File path:
flink-table/flink-table-planner/src/test/resources/org/apache/flink/table/planner/plan/nodes/exec/stream/WindowTableFunctionJsonPlanTest_jsonplan/testIndividualWindowTVFProcessingTime.out
##########
@@ -63,19 +55,16 @@
"rowtimeAttribute" : "rowtime",
"expression" : {
"rexNode" : {
- "kind" : "REX_CALL",
- "operator" : {
- "name" : "-",
- "kind" : "MINUS",
- "syntax" : "SPECIAL"
- },
+ "kind" : "CALL",
+ "syntax" : "SPECIAL",
+ "internalName" : "$-$1",
"operands" : [ {
"kind" : "INPUT_REF",
"inputIndex" : 3,
"type" : "TIMESTAMP(3)"
}, {
"kind" : "LITERAL",
- "value" : 1000,
+ "value" : "1000",
Review comment:
I guess this is the correct one now, to be a string.
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/bridging/BridgingSqlAggFunction.java
##########
@@ -117,6 +116,22 @@ public static BridgingSqlAggFunction of(
dataTypeFactory, typeFactory, kind, resolvedFunction,
typeInference);
}
+ /** Creates an instance of a aggregate function during translation. */
+ public static BridgingSqlAggFunction of(
+ FlinkContext context,
+ FlinkTypeFactory typeFactory,
+ ContextResolvedFunction resolvedFunction) {
+ final DataTypeFactory dataTypeFactory =
context.getCatalogManager().getDataTypeFactory();
Review comment:
That's only personal preference, but since we just hava a couple of
chained calls, why not inline them in the `of()` call instead of assigning them
to vars?
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/plan/nodes/exec/serde/JsonSerdeTestUtil.java
##########
@@ -54,14 +55,19 @@ static SerdeContext configuredSerdeContext() {
static SerdeContext configuredSerdeContext(
CatalogManager catalogManager, TableConfig tableConfig) {
- ClassLoader classLoader =
Thread.currentThread().getContextClassLoader();
+ final ClassLoader classLoader =
Thread.currentThread().getContextClassLoader();
- PlannerContext plannerContext =
+ final ModuleManager moduleManager = new ModuleManager();
+
Review comment:
nit: you can skip the empty lines between var declarations
--
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]
