twalthr commented on code in PR #26389:
URL: https://github.com/apache/flink/pull/26389#discussion_r2032767093
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/EnvironmentSettings.java:
##########
@@ -62,16 +65,17 @@ public class EnvironmentSettings {
private final ClassLoader classLoader;
private final @Nullable CatalogStore catalogStore;
-
- private EnvironmentSettings(Configuration configuration, ClassLoader
classLoader) {
- this(configuration, classLoader, null);
- }
+ private final @Nullable SerializationContext serializationContext;
Review Comment:
```suggestion
private final @Nullable SqlSerializationContext serializationContext;
```
In the FLIP we agreed on providing a bit more context in the naming.
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/EnvironmentSettings.java:
##########
@@ -232,12 +242,22 @@ public Builder withCatalogStore(CatalogStore
catalogStore) {
return this;
}
+ /**
+ * Provides a way to customize the process of serializing operations
to an SQL string. This
Review Comment:
```suggestion
* Provides a way to customize the process of serializing Table API
to a SQL string. This
```
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/operations/AggregateQueryOperation.java:
##########
@@ -77,23 +78,29 @@ public List<ResolvedExpression> getAggregateExpressions() {
}
@Override
- public String asSerializableString() {
- final String groupingExprs = getGroupingExprs();
+ public String asSerializableString(
+ org.apache.flink.table.operations.SerializationContext context) {
Review Comment:
Find better names that don't collide.
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/operations/QueryOperation.java:
##########
@@ -40,10 +41,12 @@ public interface QueryOperation extends Operation {
* Returns a string that fully serializes this instance. The serialized
string can be used for
* storing the query in e.g. a {@link
org.apache.flink.table.catalog.Catalog} as a view.
*
+ * @param context can be used to customize the serialization to a SQL
string
* @return detailed string for persisting in a catalog
* @see Operation#asSummaryString()
+ * @see
EnvironmentSettings.Builder#withSerializationContext(SerializationContext)
*/
- default String asSerializableString() {
+ default String asSerializableString(SerializationContext context) {
Review Comment:
This is a public evolving class. We need to keep the old
asSerializableString around.
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/ResolvedCatalogTable.java:
##########
@@ -71,8 +72,8 @@ public ResolvedSchema getResolvedSchema() {
* symmetric. The framework will resolve functions and perform other
validation tasks. A catalog
* implementation must not deal with this during a read operation.
*/
- public Map<String, String> toProperties() {
- return CatalogPropertiesUtil.serializeCatalogTable(this);
+ public Map<String, String> toProperties(SerializationContext context) {
Review Comment:
This is a public evolving class. We need to keep the old `toProperties`
around.
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/operations/WindowAggregateQueryOperation.java:
##########
@@ -117,7 +126,10 @@ public String asSerializableString() {
OperationExpressionsUtils
.scopeReferencesWithAlias(
INPUT_ALIAS, expr))
-
.map(ResolvedExpression::asSerializableString))
+ .map(
+ resolvedExpression1 ->
Review Comment:
```suggestion
e ->
```
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/operations/DefaultSerializationContext.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.flink.table.operations;
+
+import org.apache.flink.annotation.Internal;
+import org.apache.flink.table.api.TableException;
+import org.apache.flink.table.functions.FunctionDefinition;
+
+/**
+ * Default implementation of {@link SerializationContext} that throws an
exception when trying to
+ * serialize an inline function.
+ */
+@Internal
+public class DefaultSerializationContext implements SerializationContext {
+ @Override
+ public String serializeInlineFunction(FunctionDefinition
functionDefinition) {
+ throw new TableException(
Review Comment:
```suggestion
throw new ValidationException(
```
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/operations/SerializationContextAdapters.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * 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.flink.table.operations;
+
+import org.apache.flink.annotation.Internal;
+import org.apache.flink.table.expressions.SerializationContext;
+
+/** Class for adapting serialization contexts between operations and
expressions specific ones. */
+@Internal
+public class SerializationContextAdapters {
Review Comment:
Just use one unified context. Let's keep it simple.
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/catalog/ResolvedCatalogModel.java:
##########
@@ -48,11 +49,11 @@ public interface ResolvedCatalogModel extends CatalogModel {
* <p>Compared to the pure table options in {@link #getOptions()}, the map
includes input
* schema, output schema, comment and options.
*/
- Map<String, String> toProperties();
+ Map<String, String> toProperties(SerializationContext context);
Review Comment:
This is a public evolving class. We need to keep the old toProperties around.
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/EnvironmentSettings.java:
##########
@@ -62,16 +65,17 @@ public class EnvironmentSettings {
private final ClassLoader classLoader;
private final @Nullable CatalogStore catalogStore;
-
- private EnvironmentSettings(Configuration configuration, ClassLoader
classLoader) {
- this(configuration, classLoader, null);
- }
+ private final @Nullable SerializationContext serializationContext;
Review Comment:
But I'm wondering whether we can come up with a better name: How about
`SqlFactory`, `SqlBuilder`, `SqlGenerator`?
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
