You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/01/10 05:49:26 UTC
[GitHub] [iceberg] stevenzwu opened a new pull request #3865: Flink: SplitRecords for a batch of records for a single split; RecordAndPosi…
stevenzwu opened a new pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865
…tion is a mutable holder for record and position
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3865: Flink: SplitRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865#discussion_r789846685
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/source/reader/SplitRecords.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.iceberg.flink.source.reader;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.flink.annotation.Internal;
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.connector.base.source.reader.RecordsWithSplitIds;
+import org.apache.iceberg.io.CloseableIterator;
+
+/**
+ * A batch of records for one split
+ */
+@Internal
+public class SplitRecords<T> implements RecordsWithSplitIds<RecordAndPosition<T>> {
+ @Nullable
+ private final CloseableIterator<RecordAndPosition<T>> recordsForSplit;
+ private final Set<String> finishedSplits;
+
+ @Nullable
+ private String splitId;
+ @Nullable
+ private CloseableIterator<RecordAndPosition<T>> recordsForSplitCurrent;
+
+ private SplitRecords(
+ @Nullable String splitId,
+ @Nullable CloseableIterator<RecordAndPosition<T>> recordsForSplit,
+ Set<String> finishedSplits) {
+
+ this.splitId = splitId;
+ this.recordsForSplit = recordsForSplit;
+ this.finishedSplits = finishedSplits;
+ }
+
+ @Nullable
+ @Override
+ public String nextSplit() {
+ // move the split one (from current value to null)
+ String nextSplit = this.splitId;
+ this.splitId = null;
+
+ // move the iterator, from null to value (if first move) or to null (if second move)
+ this.recordsForSplitCurrent = nextSplit != null ? this.recordsForSplit : null;
+
+ return nextSplit;
+ }
+
+ @Nullable
+ @Override
+ public RecordAndPosition<T> nextRecordFromSplit() {
+ if (recordsForSplitCurrent != null) {
+ return recordsForSplitCurrent.next();
+ } else {
+ throw new IllegalStateException();
+ }
+ }
+
+ @Override
+ public void recycle() {
+ if (recordsForSplit != null) {
+ try {
+ recordsForSplit.close();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to close the record batch");
Review comment:
This should be `UncheckedIOException` instead of a generic `RuntimeException`.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3865: Flink: SplitRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865#discussion_r789860780
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/source/reader/SplitRecords.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.iceberg.flink.source.reader;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.flink.annotation.Internal;
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.connector.base.source.reader.RecordsWithSplitIds;
+import org.apache.iceberg.io.CloseableIterator;
+
+/**
+ * A batch of records for one split
+ */
+@Internal
+public class SplitRecords<T> implements RecordsWithSplitIds<RecordAndPosition<T>> {
+ @Nullable
+ private final CloseableIterator<RecordAndPosition<T>> recordsForSplit;
+ private final Set<String> finishedSplits;
+
+ @Nullable
+ private String splitId;
+ @Nullable
+ private CloseableIterator<RecordAndPosition<T>> recordsForSplitCurrent;
+
+ private SplitRecords(
+ @Nullable String splitId,
+ @Nullable CloseableIterator<RecordAndPosition<T>> recordsForSplit,
+ Set<String> finishedSplits) {
+
+ this.splitId = splitId;
+ this.recordsForSplit = recordsForSplit;
+ this.finishedSplits = finishedSplits;
+ }
+
+ @Nullable
+ @Override
+ public String nextSplit() {
+ // move the split one (from current value to null)
+ String nextSplit = this.splitId;
+ this.splitId = null;
+
+ // move the iterator, from null to value (if first move) or to null (if second move)
+ this.recordsForSplitCurrent = nextSplit != null ? this.recordsForSplit : null;
+
+ return nextSplit;
+ }
+
+ @Nullable
+ @Override
+ public RecordAndPosition<T> nextRecordFromSplit() {
+ if (recordsForSplitCurrent != null) {
+ return recordsForSplitCurrent.next();
+ } else {
+ throw new IllegalStateException();
+ }
+ }
+
+ @Override
+ public void recycle() {
+ if (recordsForSplit != null) {
+ try {
+ recordsForSplit.close();
Review comment:
Should this set state to CLOSED.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #3865: Flink: ArrayBatchRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] stevenzwu commented on a change in pull request #3865: Flink: ArrayBatchRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on a change in pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865#discussion_r797285444
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/source/reader/ArrayBatchRecords.java
##########
@@ -0,0 +1,158 @@
+/*
+ * 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.iceberg.flink.source.reader;
+
+import java.util.Collections;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.connector.base.source.reader.RecordsWithSplitIds;
+import org.apache.flink.connector.file.src.util.Pool;
+import org.apache.flink.table.data.RowData;
+import org.apache.iceberg.flink.source.DataIterator;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+/**
+ * {@link RecordsWithSplitIds} is used to pass a batch of records from fetcher to source reader.
+ * Batching is to improve the efficiency for records handover.
+ *
+ * {@link RecordsWithSplitIds} interface can encapsulate batches from multiple splits.
+ * This is the case for Kafka source where fetchers can retrieve records from multiple
+ * Kafka partitions at the same time.
+ *
+ * For file-based sources like Iceberg, readers always read one split/file at a time.
+ * Hence, we will only have a batch of records for one split here.
+ *
+ * This class uses array to store a batch of records from the same file (with the same fileOffset).
+ */
+class ArrayBatchRecords<T> implements RecordsWithSplitIds<RecordAndPosition<T>> {
+ @Nullable
+ private String splitId;
+ @Nullable
+ private final Pool.Recycler<T[]> recycler;
+ @Nullable
+ private final T[] records;
+ private final int numberOfRecords;
+ private final Set<String> finishedSplits;
+ private final RecordAndPosition<T> recordAndPosition;
+
+ // point to current read position within the records array
+ private int position;
+
+ private ArrayBatchRecords(
+ @Nullable String splitId, @Nullable Pool.Recycler<T[]> recycler, @Nullable T[] records,
+ int numberOfRecords, int fileOffset, long startingRecordOffset, Set<String> finishedSplits) {
+ Preconditions.checkArgument(numberOfRecords >= 0, "numberOfRecords can't be negative");
+ Preconditions.checkArgument(fileOffset >= 0, "fileOffset can't be negative");
+ Preconditions.checkArgument(startingRecordOffset >= 0, "numberOfRecords can't be negative");
+
+ this.splitId = splitId;
+ this.recycler = recycler;
+ this.records = records;
+ this.numberOfRecords = numberOfRecords;
+ this.finishedSplits = Preconditions.checkNotNull(finishedSplits, "finishedSplits can be empty but not null");
+ this.recordAndPosition = new RecordAndPosition<>();
+
+ recordAndPosition.set(null, fileOffset, startingRecordOffset);
+ this.position = 0;
+ }
+
+ @Nullable
+ @Override
+ public String nextSplit() {
+ String nextSplit = this.splitId;
+ // set the splitId to null to indicate no more splits
+ // this class only contains record for one split
+ this.splitId = null;
+ return nextSplit;
+ }
+
+ @Nullable
+ @Override
+ public RecordAndPosition<T> nextRecordFromSplit() {
+ if (position < numberOfRecords) {
+ recordAndPosition.record(records[position++]);
+ return recordAndPosition;
+ } else {
+ return null;
+ }
+ }
+
+ /**
+ * This method is called when all records from this batch has been emitted.
+ * If recycler is set, it should be called to return the records array back to pool.
+ */
+ @Override
+ public void recycle() {
+ if (recycler != null) {
+ recycler.recycle(records);
+ }
+ }
+
+ @Override
+ public Set<String> finishedSplits() {
+ return finishedSplits;
+ }
+
+ @VisibleForTesting
+ T[] records() {
+ return records;
+ }
+
+ @VisibleForTesting
+ int numberOfRecords() {
+ return numberOfRecords;
+ }
+
+ /**
+ * Create a ArrayBatchRecords backed up an array with records from the same file
+ *
+ * @param splitId Iceberg source only read from one split a time.
+ * We never have multiple records from multiple splits.
+ * @param recycler Because {@link DataIterator} with {@link RowData} returns an iterator of reused RowData object,
+ * we need to clone RowData eagerly when constructing a batch fo records.
Review comment:
good batch. fixed
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3865: Flink: SplitRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865#discussion_r789858258
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/source/reader/SplitRecords.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.iceberg.flink.source.reader;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.flink.annotation.Internal;
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.connector.base.source.reader.RecordsWithSplitIds;
+import org.apache.iceberg.io.CloseableIterator;
+
+/**
+ * A batch of records for one split
+ */
+@Internal
+public class SplitRecords<T> implements RecordsWithSplitIds<RecordAndPosition<T>> {
+ @Nullable
+ private final CloseableIterator<RecordAndPosition<T>> recordsForSplit;
+ private final Set<String> finishedSplits;
+
+ @Nullable
+ private String splitId;
+ @Nullable
+ private CloseableIterator<RecordAndPosition<T>> recordsForSplitCurrent;
+
+ private SplitRecords(
+ @Nullable String splitId,
+ @Nullable CloseableIterator<RecordAndPosition<T>> recordsForSplit,
+ Set<String> finishedSplits) {
+
+ this.splitId = splitId;
+ this.recordsForSplit = recordsForSplit;
+ this.finishedSplits = finishedSplits;
+ }
+
+ @Nullable
+ @Override
+ public String nextSplit() {
+ // move the split one (from current value to null)
+ String nextSplit = this.splitId;
+ this.splitId = null;
+
+ // move the iterator, from null to value (if first move) or to null (if second move)
+ this.recordsForSplitCurrent = nextSplit != null ? this.recordsForSplit : null;
+
+ return nextSplit;
+ }
+
+ @Nullable
+ @Override
+ public RecordAndPosition<T> nextRecordFromSplit() {
+ if (recordsForSplitCurrent != null) {
+ return recordsForSplitCurrent.next();
Review comment:
Does this need to call `hasNext`?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3865: Flink: SplitRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865#discussion_r789861047
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/source/reader/SplitRecords.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.iceberg.flink.source.reader;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.flink.annotation.Internal;
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.connector.base.source.reader.RecordsWithSplitIds;
+import org.apache.iceberg.io.CloseableIterator;
+
+/**
+ * A batch of records for one split
+ */
+@Internal
+public class SplitRecords<T> implements RecordsWithSplitIds<RecordAndPosition<T>> {
+ @Nullable
+ private final CloseableIterator<RecordAndPosition<T>> recordsForSplit;
+ private final Set<String> finishedSplits;
+
+ @Nullable
+ private String splitId;
+ @Nullable
+ private CloseableIterator<RecordAndPosition<T>> recordsForSplitCurrent;
+
+ private SplitRecords(
+ @Nullable String splitId,
+ @Nullable CloseableIterator<RecordAndPosition<T>> recordsForSplit,
+ Set<String> finishedSplits) {
+
+ this.splitId = splitId;
+ this.recordsForSplit = recordsForSplit;
+ this.finishedSplits = finishedSplits;
+ }
+
+ @Nullable
+ @Override
+ public String nextSplit() {
+ // move the split one (from current value to null)
+ String nextSplit = this.splitId;
+ this.splitId = null;
+
+ // move the iterator, from null to value (if first move) or to null (if second move)
+ this.recordsForSplitCurrent = nextSplit != null ? this.recordsForSplit : null;
+
+ return nextSplit;
+ }
+
+ @Nullable
+ @Override
+ public RecordAndPosition<T> nextRecordFromSplit() {
+ if (recordsForSplitCurrent != null) {
+ return recordsForSplitCurrent.next();
+ } else {
+ throw new IllegalStateException();
+ }
+ }
+
+ @Override
+ public void recycle() {
+ if (recordsForSplit != null) {
+ try {
+ recordsForSplit.close();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to close the record batch");
+ }
+ }
+ }
+
+ @Override
+ public Set<String> finishedSplits() {
+ return finishedSplits;
+ }
+
Review comment:
This should return `Sets.newHashSet(splitId)` when state is CLOSED?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3865: Flink: ArrayBatchRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865#discussion_r799656225
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/source/reader/ArrayBatchRecords.java
##########
@@ -0,0 +1,158 @@
+/*
+ * 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.iceberg.flink.source.reader;
+
+import java.util.Collections;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.connector.base.source.reader.RecordsWithSplitIds;
+import org.apache.flink.connector.file.src.util.Pool;
+import org.apache.flink.table.data.RowData;
+import org.apache.iceberg.flink.source.DataIterator;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+/**
+ * {@link RecordsWithSplitIds} is used to pass a batch of records from fetcher to source reader.
+ * Batching is to improve the efficiency for records handover.
+ *
+ * {@link RecordsWithSplitIds} interface can encapsulate batches from multiple splits.
+ * This is the case for Kafka source where fetchers can retrieve records from multiple
+ * Kafka partitions at the same time.
+ *
+ * For file-based sources like Iceberg, readers always read one split/file at a time.
+ * Hence, we will only have a batch of records for one split here.
+ *
+ * This class uses array to store a batch of records from the same file (with the same fileOffset).
+ */
+class ArrayBatchRecords<T> implements RecordsWithSplitIds<RecordAndPosition<T>> {
+ @Nullable
+ private String splitId;
+ @Nullable
+ private final Pool.Recycler<T[]> recycler;
+ @Nullable
+ private final T[] records;
+ private final int numberOfRecords;
+ private final Set<String> finishedSplits;
+ private final RecordAndPosition<T> recordAndPosition;
+
+ // point to current read position within the records array
+ private int position;
+
+ private ArrayBatchRecords(
+ @Nullable String splitId, @Nullable Pool.Recycler<T[]> recycler, @Nullable T[] records,
+ int numberOfRecords, int fileOffset, long startingRecordOffset, Set<String> finishedSplits) {
+ Preconditions.checkArgument(numberOfRecords >= 0, "numberOfRecords can't be negative");
+ Preconditions.checkArgument(fileOffset >= 0, "fileOffset can't be negative");
+ Preconditions.checkArgument(startingRecordOffset >= 0, "numberOfRecords can't be negative");
+
+ this.splitId = splitId;
+ this.recycler = recycler;
+ this.records = records;
+ this.numberOfRecords = numberOfRecords;
+ this.finishedSplits = Preconditions.checkNotNull(finishedSplits, "finishedSplits can be empty but not null");
+ this.recordAndPosition = new RecordAndPosition<>();
+
+ recordAndPosition.set(null, fileOffset, startingRecordOffset);
+ this.position = 0;
+ }
+
+ @Nullable
+ @Override
+ public String nextSplit() {
+ String nextSplit = this.splitId;
+ // set the splitId to null to indicate no more splits
+ // this class only contains record for one split
+ this.splitId = null;
+ return nextSplit;
+ }
+
+ @Nullable
+ @Override
+ public RecordAndPosition<T> nextRecordFromSplit() {
+ if (position < numberOfRecords) {
+ recordAndPosition.record(records[position++]);
Review comment:
Can you increment/decrement on a separate line? We don't use the return value of ++ because it is hard to reason about.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3865: Flink: SplitRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865#discussion_r789864891
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/source/reader/SplitRecords.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.iceberg.flink.source.reader;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.flink.annotation.Internal;
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.connector.base.source.reader.RecordsWithSplitIds;
+import org.apache.iceberg.io.CloseableIterator;
+
+/**
+ * A batch of records for one split
+ */
+@Internal
+public class SplitRecords<T> implements RecordsWithSplitIds<RecordAndPosition<T>> {
+ @Nullable
+ private final CloseableIterator<RecordAndPosition<T>> recordsForSplit;
+ private final Set<String> finishedSplits;
+
+ @Nullable
+ private String splitId;
+ @Nullable
+ private CloseableIterator<RecordAndPosition<T>> recordsForSplitCurrent;
+
+ private SplitRecords(
+ @Nullable String splitId,
+ @Nullable CloseableIterator<RecordAndPosition<T>> recordsForSplit,
+ Set<String> finishedSplits) {
+
+ this.splitId = splitId;
+ this.recordsForSplit = recordsForSplit;
+ this.finishedSplits = finishedSplits;
+ }
+
+ @Nullable
+ @Override
+ public String nextSplit() {
+ // move the split one (from current value to null)
+ String nextSplit = this.splitId;
+ this.splitId = null;
+
+ // move the iterator, from null to value (if first move) or to null (if second move)
+ this.recordsForSplitCurrent = nextSplit != null ? this.recordsForSplit : null;
+
+ return nextSplit;
+ }
+
+ @Nullable
+ @Override
+ public RecordAndPosition<T> nextRecordFromSplit() {
+ if (recordsForSplitCurrent != null) {
+ return recordsForSplitCurrent.next();
+ } else {
+ throw new IllegalStateException();
+ }
+ }
+
+ @Override
+ public void recycle() {
+ if (recordsForSplit != null) {
+ try {
+ recordsForSplit.close();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to close the record batch");
+ }
+ }
+ }
+
+ @Override
+ public Set<String> finishedSplits() {
+ return finishedSplits;
+ }
+
Review comment:
If not, we need to explain why the split isn't actually finished at this point.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3865: Flink: SplitRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865#discussion_r789859651
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/source/reader/SplitRecords.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.iceberg.flink.source.reader;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.flink.annotation.Internal;
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.connector.base.source.reader.RecordsWithSplitIds;
+import org.apache.iceberg.io.CloseableIterator;
+
+/**
+ * A batch of records for one split
+ */
+@Internal
+public class SplitRecords<T> implements RecordsWithSplitIds<RecordAndPosition<T>> {
+ @Nullable
+ private final CloseableIterator<RecordAndPosition<T>> recordsForSplit;
+ private final Set<String> finishedSplits;
+
+ @Nullable
+ private String splitId;
+ @Nullable
+ private CloseableIterator<RecordAndPosition<T>> recordsForSplitCurrent;
+
+ private SplitRecords(
+ @Nullable String splitId,
+ @Nullable CloseableIterator<RecordAndPosition<T>> recordsForSplit,
+ Set<String> finishedSplits) {
+
+ this.splitId = splitId;
+ this.recordsForSplit = recordsForSplit;
+ this.finishedSplits = finishedSplits;
+ }
+
+ @Nullable
+ @Override
+ public String nextSplit() {
+ // move the split one (from current value to null)
+ String nextSplit = this.splitId;
+ this.splitId = null;
+
+ // move the iterator, from null to value (if first move) or to null (if second move)
+ this.recordsForSplitCurrent = nextSplit != null ? this.recordsForSplit : null;
+
+ return nextSplit;
+ }
+
+ @Nullable
+ @Override
+ public RecordAndPosition<T> nextRecordFromSplit() {
+ if (recordsForSplitCurrent != null) {
+ return recordsForSplitCurrent.next();
+ } else {
+ throw new IllegalStateException();
+ }
+ }
+
+ @Override
+ public void recycle() {
Review comment:
If this is `close`, then could you add a comment to clarify what the purpose of this method is?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3865: Flink: ArrayBatchRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865#discussion_r796205687
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/source/reader/ArrayBatchRecords.java
##########
@@ -0,0 +1,158 @@
+/*
+ * 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.iceberg.flink.source.reader;
+
+import java.util.Collections;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.connector.base.source.reader.RecordsWithSplitIds;
+import org.apache.flink.connector.file.src.util.Pool;
+import org.apache.flink.table.data.RowData;
+import org.apache.iceberg.flink.source.DataIterator;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+/**
+ * {@link RecordsWithSplitIds} is used to pass a batch of records from fetcher to source reader.
+ * Batching is to improve the efficiency for records handover.
+ *
+ * {@link RecordsWithSplitIds} interface can encapsulate batches from multiple splits.
+ * This is the case for Kafka source where fetchers can retrieve records from multiple
+ * Kafka partitions at the same time.
+ *
+ * For file-based sources like Iceberg, readers always read one split/file at a time.
+ * Hence, we will only have a batch of records for one split here.
+ *
+ * This class uses array to store a batch of records from the same file (with the same fileOffset).
+ */
+class ArrayBatchRecords<T> implements RecordsWithSplitIds<RecordAndPosition<T>> {
+ @Nullable
+ private String splitId;
+ @Nullable
+ private final Pool.Recycler<T[]> recycler;
+ @Nullable
+ private final T[] records;
+ private final int numberOfRecords;
+ private final Set<String> finishedSplits;
+ private final RecordAndPosition<T> recordAndPosition;
+
+ // point to current read position within the records array
+ private int position;
+
+ private ArrayBatchRecords(
+ @Nullable String splitId, @Nullable Pool.Recycler<T[]> recycler, @Nullable T[] records,
+ int numberOfRecords, int fileOffset, long startingRecordOffset, Set<String> finishedSplits) {
+ Preconditions.checkArgument(numberOfRecords >= 0, "numberOfRecords can't be negative");
+ Preconditions.checkArgument(fileOffset >= 0, "fileOffset can't be negative");
+ Preconditions.checkArgument(startingRecordOffset >= 0, "numberOfRecords can't be negative");
+
+ this.splitId = splitId;
+ this.recycler = recycler;
+ this.records = records;
+ this.numberOfRecords = numberOfRecords;
+ this.finishedSplits = Preconditions.checkNotNull(finishedSplits, "finishedSplits can be empty but not null");
+ this.recordAndPosition = new RecordAndPosition<>();
+
+ recordAndPosition.set(null, fileOffset, startingRecordOffset);
+ this.position = 0;
+ }
+
+ @Nullable
+ @Override
+ public String nextSplit() {
+ String nextSplit = this.splitId;
+ // set the splitId to null to indicate no more splits
+ // this class only contains record for one split
+ this.splitId = null;
+ return nextSplit;
+ }
+
+ @Nullable
+ @Override
+ public RecordAndPosition<T> nextRecordFromSplit() {
+ if (position < numberOfRecords) {
+ recordAndPosition.record(records[position++]);
+ return recordAndPosition;
+ } else {
+ return null;
+ }
+ }
+
+ /**
+ * This method is called when all records from this batch has been emitted.
+ * If recycler is set, it should be called to return the records array back to pool.
+ */
+ @Override
+ public void recycle() {
+ if (recycler != null) {
+ recycler.recycle(records);
+ }
+ }
+
+ @Override
+ public Set<String> finishedSplits() {
+ return finishedSplits;
+ }
+
+ @VisibleForTesting
+ T[] records() {
+ return records;
+ }
+
+ @VisibleForTesting
+ int numberOfRecords() {
+ return numberOfRecords;
+ }
+
+ /**
+ * Create a ArrayBatchRecords backed up an array with records from the same file
+ *
+ * @param splitId Iceberg source only read from one split a time.
+ * We never have multiple records from multiple splits.
+ * @param recycler Because {@link DataIterator} with {@link RowData} returns an iterator of reused RowData object,
+ * we need to clone RowData eagerly when constructing a batch fo records.
Review comment:
Nit: Typo `fo` instead of `of` in `batch of records`.
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3865: Flink: ArrayBatchRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865#discussion_r799663125
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/source/reader/ArrayBatchRecords.java
##########
@@ -0,0 +1,158 @@
+/*
+ * 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.iceberg.flink.source.reader;
+
+import java.util.Collections;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.connector.base.source.reader.RecordsWithSplitIds;
+import org.apache.flink.connector.file.src.util.Pool;
+import org.apache.flink.table.data.RowData;
+import org.apache.iceberg.flink.source.DataIterator;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+/**
+ * {@link RecordsWithSplitIds} is used to pass a batch of records from fetcher to source reader.
+ * Batching is to improve the efficiency for records handover.
+ *
+ * {@link RecordsWithSplitIds} interface can encapsulate batches from multiple splits.
+ * This is the case for Kafka source where fetchers can retrieve records from multiple
+ * Kafka partitions at the same time.
+ *
+ * For file-based sources like Iceberg, readers always read one split/file at a time.
+ * Hence, we will only have a batch of records for one split here.
+ *
+ * This class uses array to store a batch of records from the same file (with the same fileOffset).
+ */
+class ArrayBatchRecords<T> implements RecordsWithSplitIds<RecordAndPosition<T>> {
+ @Nullable
+ private String splitId;
+ @Nullable
+ private final Pool.Recycler<T[]> recycler;
+ @Nullable
+ private final T[] records;
+ private final int numberOfRecords;
+ private final Set<String> finishedSplits;
+ private final RecordAndPosition<T> recordAndPosition;
+
+ // point to current read position within the records array
+ private int position;
+
+ private ArrayBatchRecords(
+ @Nullable String splitId, @Nullable Pool.Recycler<T[]> recycler, @Nullable T[] records,
+ int numberOfRecords, int fileOffset, long startingRecordOffset, Set<String> finishedSplits) {
+ Preconditions.checkArgument(numberOfRecords >= 0, "numberOfRecords can't be negative");
+ Preconditions.checkArgument(fileOffset >= 0, "fileOffset can't be negative");
+ Preconditions.checkArgument(startingRecordOffset >= 0, "numberOfRecords can't be negative");
+
+ this.splitId = splitId;
+ this.recycler = recycler;
+ this.records = records;
+ this.numberOfRecords = numberOfRecords;
+ this.finishedSplits = Preconditions.checkNotNull(finishedSplits, "finishedSplits can be empty but not null");
+ this.recordAndPosition = new RecordAndPosition<>();
+
+ recordAndPosition.set(null, fileOffset, startingRecordOffset);
+ this.position = 0;
+ }
+
+ @Nullable
+ @Override
+ public String nextSplit() {
+ String nextSplit = this.splitId;
+ // set the splitId to null to indicate no more splits
+ // this class only contains record for one split
+ this.splitId = null;
+ return nextSplit;
+ }
+
+ @Nullable
+ @Override
+ public RecordAndPosition<T> nextRecordFromSplit() {
+ if (position < numberOfRecords) {
Review comment:
You may want to use `if (splitId != null && position < numberOfRecords)`
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3865: Flink: SplitRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865#discussion_r789860478
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/source/reader/SplitRecords.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.iceberg.flink.source.reader;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.flink.annotation.Internal;
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.connector.base.source.reader.RecordsWithSplitIds;
+import org.apache.iceberg.io.CloseableIterator;
+
+/**
+ * A batch of records for one split
+ */
+@Internal
+public class SplitRecords<T> implements RecordsWithSplitIds<RecordAndPosition<T>> {
+ @Nullable
+ private final CloseableIterator<RecordAndPosition<T>> recordsForSplit;
+ private final Set<String> finishedSplits;
+
+ @Nullable
+ private String splitId;
+ @Nullable
+ private CloseableIterator<RecordAndPosition<T>> recordsForSplitCurrent;
+
+ private SplitRecords(
+ @Nullable String splitId,
+ @Nullable CloseableIterator<RecordAndPosition<T>> recordsForSplit,
+ Set<String> finishedSplits) {
+
+ this.splitId = splitId;
+ this.recordsForSplit = recordsForSplit;
+ this.finishedSplits = finishedSplits;
+ }
+
+ @Nullable
+ @Override
+ public String nextSplit() {
+ // move the split one (from current value to null)
+ String nextSplit = this.splitId;
+ this.splitId = null;
+
+ // move the iterator, from null to value (if first move) or to null (if second move)
+ this.recordsForSplitCurrent = nextSplit != null ? this.recordsForSplit : null;
+
+ return nextSplit;
+ }
+
+ @Nullable
+ @Override
+ public RecordAndPosition<T> nextRecordFromSplit() {
+ if (recordsForSplitCurrent != null) {
+ return recordsForSplitCurrent.next();
+ } else {
+ throw new IllegalStateException();
+ }
+ }
+
+ @Override
+ public void recycle() {
+ if (recordsForSplit != null) {
+ try {
+ recordsForSplit.close();
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to close the record batch");
+ }
+ }
+ }
+
+ @Override
+ public Set<String> finishedSplits() {
+ return finishedSplits;
+ }
+
+ @VisibleForTesting
+ CloseableIterator<RecordAndPosition<T>> recordsForSplitCurrent() {
+ return recordsForSplitCurrent;
+ }
+
+ public static <T> SplitRecords<T> forRecords(
+ String splitId, CloseableIterator<RecordAndPosition<T>> recordsForSplit) {
+ return new SplitRecords<>(splitId, recordsForSplit, Collections.emptySet());
+ }
+
+ public static <T> SplitRecords<T> finishedSplit(String splitId) {
+ return new SplitRecords<>(null, null, Collections.singleton(splitId));
Review comment:
Shoudl this produce a split with state CLOSED, no iterator, and the split ID?
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] stevenzwu commented on a change in pull request #3865: Flink: ArrayBatchRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on a change in pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865#discussion_r794203680
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/source/reader/ArrayBatchRecords.java
##########
@@ -0,0 +1,158 @@
+/*
+ * 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.iceberg.flink.source.reader;
+
+import java.util.Collections;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.connector.base.source.reader.RecordsWithSplitIds;
+import org.apache.flink.connector.file.src.util.Pool;
+import org.apache.flink.table.data.RowData;
+import org.apache.iceberg.flink.source.DataIterator;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+
+/**
+ * {@link RecordsWithSplitIds} is used to pass a batch of records from fetcher to source reader.
+ * Batching is to improve the efficiency for records handover.
+ *
+ * {@link RecordsWithSplitIds} interface can encapsulate batches from multiple splits.
+ * This is the case for Kafka source where fetchers can retrieve records from multiple
+ * Kafka partitions at the same time.
+ *
+ * For file-based sources like Iceberg, readers always read one split/file at a time.
+ * Hence, we will only have a batch of records for one split here.
+ *
+ * This class uses array to store a batch of records from the same file (with the same fileOffset).
+ */
+class ArrayBatchRecords<T> implements RecordsWithSplitIds<RecordAndPosition<T>> {
Review comment:
this class is used here: https://github.com/stevenzwu/iceberg/blob/flip27IcebergSourceStaging/flink/v1.13/flink/src/main/java/org/apache/iceberg/flink/source/reader/ArrayPoolDataIteratorBatcher.java#L93
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3865: Flink: SplitRecords for a batch of records for a single file; RecordAndPosi…
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3865:
URL: https://github.com/apache/iceberg/pull/3865#discussion_r789857578
##########
File path: flink/v1.14/flink/src/main/java/org/apache/iceberg/flink/source/reader/SplitRecords.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.iceberg.flink.source.reader;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import javax.annotation.Nullable;
+import org.apache.flink.annotation.Internal;
+import org.apache.flink.annotation.VisibleForTesting;
+import org.apache.flink.connector.base.source.reader.RecordsWithSplitIds;
+import org.apache.iceberg.io.CloseableIterator;
+
+/**
+ * A batch of records for one split
+ */
+@Internal
+public class SplitRecords<T> implements RecordsWithSplitIds<RecordAndPosition<T>> {
Review comment:
```java
enum SplitRecordsState {
NEW,
OPEN,
CLOSED
}
```
```java
public String nextSplit() {
switch (splitState) {
case NEW:
this.splitState = OPEN;
return splitId;
default:
this.splitState = CLOSED;
return null;
}
}
```
--
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@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org