You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/10/24 10:08:18 UTC
[GitHub] [flink-table-store] JingsongLi opened a new pull request, #330: [FLINK-29736] Abstract a table interface for both data and metadata tables
JingsongLi opened a new pull request, #330:
URL: https://github.com/apache/flink-table-store/pull/330
Currently, FileStoreTable is only for data tables, we can create a new interface Table for all tables.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] LadyForest commented on a diff in pull request #330: [FLINK-29736] Abstract a table interface for both data and metadata tables
Posted by GitBox <gi...@apache.org>.
LadyForest commented on code in PR #330:
URL: https://github.com/apache/flink-table-store/pull/330#discussion_r1004094344
##########
flink-table-store-core/src/main/java/org/apache/flink/table/store/table/source/DataTableScan.java:
##########
@@ -0,0 +1,163 @@
+/*
+ * 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.store.table.source;
+
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.table.data.binary.BinaryRowData;
+import org.apache.flink.table.store.file.io.DataFileMeta;
+import org.apache.flink.table.store.file.operation.FileStoreScan;
+import org.apache.flink.table.store.file.predicate.Predicate;
+import org.apache.flink.table.store.file.predicate.PredicateBuilder;
+import org.apache.flink.table.store.file.schema.TableSchema;
+import org.apache.flink.table.store.file.utils.FileStorePathFactory;
+
+import javax.annotation.Nullable;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+import static org.apache.flink.table.store.file.predicate.PredicateBuilder.transformFieldMapping;
+
+/** An abstraction layer above {@link FileStoreScan} to provide input split generation. */
+public abstract class DataTableScan implements TableScan {
+
+ private final FileStoreScan scan;
+ private final TableSchema tableSchema;
+ private final FileStorePathFactory pathFactory;
+
+ private boolean isIncremental = false;
+
+ protected DataTableScan(
+ FileStoreScan scan, TableSchema tableSchema, FileStorePathFactory pathFactory) {
+ this.scan = scan;
+ this.tableSchema = tableSchema;
+ this.pathFactory = pathFactory;
+ }
+
+ public DataTableScan withSnapshot(long snapshotId) {
+ scan.withSnapshot(snapshotId);
+ return this;
+ }
+
+ public DataTableScan withFilter(List<Predicate> predicates) {
+ if (predicates == null || predicates.isEmpty()) {
+ return this;
+ }
+ return withFilter(PredicateBuilder.and(predicates));
+ }
Review Comment:
Is there any concern not to use super interface's `withFilter(List<Predicate> predicates)`?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] LadyForest commented on a diff in pull request #330: [FLINK-29736] Abstract a table interface for both data and metadata tables
Posted by GitBox <gi...@apache.org>.
LadyForest commented on code in PR #330:
URL: https://github.com/apache/flink-table-store/pull/330#discussion_r1004123082
##########
flink-table-store-spark/src/main/java/org/apache/flink/table/store/spark/SparkTable.java:
##########
@@ -33,12 +32,12 @@
import java.util.HashSet;
import java.util.Set;
-/** A spark {@link Table} for table store. */
+/** A spark {@code Table} for table store. */
Review Comment:
Why change it?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] JingsongLi commented on a diff in pull request #330: [FLINK-29736] Abstract a table interface for both data and metadata tables
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on code in PR #330:
URL: https://github.com/apache/flink-table-store/pull/330#discussion_r1004179512
##########
flink-table-store-spark/src/main/java/org/apache/flink/table/store/spark/SparkTable.java:
##########
@@ -33,12 +32,12 @@
import java.util.HashSet;
import java.util.Set;
-/** A spark {@link Table} for table store. */
+/** A spark {@code Table} for table store. */
Review Comment:
I will change it to `org.apache.spark.sql.connector.catalog.Table`
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] LadyForest commented on a diff in pull request #330: [FLINK-29736] Abstract a table interface for both data and metadata tables
Posted by GitBox <gi...@apache.org>.
LadyForest commented on code in PR #330:
URL: https://github.com/apache/flink-table-store/pull/330#discussion_r1004131903
##########
flink-table-store-spark/src/main/java/org/apache/flink/table/store/spark/SparkScan.java:
##########
@@ -128,13 +124,13 @@ public boolean equals(Object o) {
}
SparkScan that = (SparkScan) o;
- return table.location().equals(that.table.location())
+ return table.name().equals(that.table.name())
Review Comment:
Is it safe to just compare table's name? What about two tables with the same name and schema but under different namespace?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] JingsongLi commented on a diff in pull request #330: [FLINK-29736] Abstract a table interface for both data and metadata tables
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on code in PR #330:
URL: https://github.com/apache/flink-table-store/pull/330#discussion_r1004181056
##########
flink-table-store-spark/src/main/java/org/apache/flink/table/store/spark/SparkScan.java:
##########
@@ -128,13 +124,13 @@ public boolean equals(Object o) {
}
SparkScan that = (SparkScan) o;
- return table.location().equals(that.table.location())
+ return table.name().equals(that.table.name())
Review Comment:
In one catalog, different tables should have different names.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] JingsongLi commented on a diff in pull request #330: [FLINK-29736] Abstract a table interface for both data and metadata tables
Posted by GitBox <gi...@apache.org>.
JingsongLi commented on code in PR #330:
URL: https://github.com/apache/flink-table-store/pull/330#discussion_r1004178866
##########
flink-table-store-core/src/main/java/org/apache/flink/table/store/table/source/DataTableScan.java:
##########
@@ -0,0 +1,163 @@
+/*
+ * 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.store.table.source;
+
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.table.data.binary.BinaryRowData;
+import org.apache.flink.table.store.file.io.DataFileMeta;
+import org.apache.flink.table.store.file.operation.FileStoreScan;
+import org.apache.flink.table.store.file.predicate.Predicate;
+import org.apache.flink.table.store.file.predicate.PredicateBuilder;
+import org.apache.flink.table.store.file.schema.TableSchema;
+import org.apache.flink.table.store.file.utils.FileStorePathFactory;
+
+import javax.annotation.Nullable;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+import static org.apache.flink.table.store.file.predicate.PredicateBuilder.transformFieldMapping;
+
+/** An abstraction layer above {@link FileStoreScan} to provide input split generation. */
+public abstract class DataTableScan implements TableScan {
+
+ private final FileStoreScan scan;
+ private final TableSchema tableSchema;
+ private final FileStorePathFactory pathFactory;
+
+ private boolean isIncremental = false;
+
+ protected DataTableScan(
+ FileStoreScan scan, TableSchema tableSchema, FileStorePathFactory pathFactory) {
+ this.scan = scan;
+ this.tableSchema = tableSchema;
+ this.pathFactory = pathFactory;
+ }
+
+ public DataTableScan withSnapshot(long snapshotId) {
+ scan.withSnapshot(snapshotId);
+ return this;
+ }
+
+ public DataTableScan withFilter(List<Predicate> predicates) {
+ if (predicates == null || predicates.isEmpty()) {
+ return this;
+ }
+ return withFilter(PredicateBuilder.and(predicates));
+ }
Review Comment:
No, let's use it.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] JingsongLi merged pull request #330: [FLINK-29736] Abstract a table interface for both data and metadata tables
Posted by GitBox <gi...@apache.org>.
JingsongLi merged PR #330:
URL: https://github.com/apache/flink-table-store/pull/330
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] LadyForest commented on a diff in pull request #330: [FLINK-29736] Abstract a table interface for both data and metadata tables
Posted by GitBox <gi...@apache.org>.
LadyForest commented on code in PR #330:
URL: https://github.com/apache/flink-table-store/pull/330#discussion_r1004260736
##########
flink-table-store-core/src/main/java/org/apache/flink/table/store/table/source/TableStreamingReader.java:
##########
@@ -84,11 +84,11 @@ public TableStreamingReader(
@Nullable
public Iterator<RowData> nextBatch() throws IOException {
if (enumerator == null) {
- TableScan scan = table.newScan();
+ DataTableScan scan = (DataTableScan) table.newScan();
Review Comment:
Remove redundant cast?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] LadyForest commented on a diff in pull request #330: [FLINK-29736] Abstract a table interface for both data and metadata tables
Posted by GitBox <gi...@apache.org>.
LadyForest commented on code in PR #330:
URL: https://github.com/apache/flink-table-store/pull/330#discussion_r1004061771
##########
flink-table-store-core/src/main/java/org/apache/flink/table/store/file/catalog/Catalog.java:
##########
@@ -116,7 +117,7 @@ void dropDatabase(String name, boolean ignoreIfNotExists, boolean cascade)
* @return The requested table
* @throws TableNotExistException if the target does not exist
*/
- default FileStoreTable getTable(ObjectPath tablePath) throws TableNotExistException {
+ default Table getTable(ObjectPath tablePath) throws TableNotExistException {
Review Comment:
Better to update the method description at L#114
```
Return a {@link Table} identified by ...
```
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink-table-store] LadyForest commented on a diff in pull request #330: [FLINK-29736] Abstract a table interface for both data and metadata tables
Posted by GitBox <gi...@apache.org>.
LadyForest commented on code in PR #330:
URL: https://github.com/apache/flink-table-store/pull/330#discussion_r1004095740
##########
flink-table-store-core/src/main/java/org/apache/flink/table/store/table/source/DataTableScan.java:
##########
@@ -0,0 +1,163 @@
+/*
+ * 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.store.table.source;
+
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.table.data.binary.BinaryRowData;
+import org.apache.flink.table.store.file.io.DataFileMeta;
+import org.apache.flink.table.store.file.operation.FileStoreScan;
+import org.apache.flink.table.store.file.predicate.Predicate;
+import org.apache.flink.table.store.file.predicate.PredicateBuilder;
+import org.apache.flink.table.store.file.schema.TableSchema;
+import org.apache.flink.table.store.file.utils.FileStorePathFactory;
+
+import javax.annotation.Nullable;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+import static org.apache.flink.table.store.file.predicate.PredicateBuilder.transformFieldMapping;
+
+/** An abstraction layer above {@link FileStoreScan} to provide input split generation. */
+public abstract class DataTableScan implements TableScan {
+
+ private final FileStoreScan scan;
+ private final TableSchema tableSchema;
+ private final FileStorePathFactory pathFactory;
+
+ private boolean isIncremental = false;
+
+ protected DataTableScan(
+ FileStoreScan scan, TableSchema tableSchema, FileStorePathFactory pathFactory) {
+ this.scan = scan;
+ this.tableSchema = tableSchema;
+ this.pathFactory = pathFactory;
+ }
+
+ public DataTableScan withSnapshot(long snapshotId) {
+ scan.withSnapshot(snapshotId);
+ return this;
+ }
+
+ public DataTableScan withFilter(List<Predicate> predicates) {
+ if (predicates == null || predicates.isEmpty()) {
+ return this;
+ }
+ return withFilter(PredicateBuilder.and(predicates));
+ }
+
+ public DataTableScan withFilter(Predicate predicate) {
Review Comment:
Add `@Override` annotation?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org