zeroshade commented on code in PR #1939: URL: https://github.com/apache/iceberg-go/pull/1939#discussion_r3883292251
########## table/scan_splits.go: ########## @@ -0,0 +1,70 @@ +// 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 table + +import "github.com/apache/iceberg-go" + +// splitParquetScanTask returns one task per valid Parquet split offset when a +// complete, large-file task can be split safely. The boolean is false when the +// original task should be used unchanged. Split offsets are absolute byte +// positions in the data file and each resulting range ends at the next offset, +// or at the end of the file. +// +// Tasks that are already partial, small, non-Parquet, or missing usable split +// offsets stay unchanged. Keeping the original task in those cases preserves +// the existing behavior for remote tasks and older files. +func splitParquetScanTask(task FileScanTask, targetSize int64) ([]FileScanTask, bool) { + file := task.File + if file == nil || file.FileFormat() != iceberg.ParquetFile || targetSize <= 0 { + return nil, false + } + + fileSize := file.FileSizeBytes() + if fileSize <= targetSize || task.Start != 0 || task.Length != fileSize { + return nil, false + } + + offsets := file.SplitOffsets() + if len(offsets) < 2 { + return nil, false + } + + for i, offset := range offsets { + if offset < 0 || offset >= fileSize || (i > 0 && offset <= offsets[i-1]) { + return nil, false + } + } + + result := make([]FileScanTask, 0, len(offsets)) + for i, start := range offsets { Review Comment: **[P1] Sparse valid split offsets silently drop rows.** Iceberg requires `split_offsets` to be sorted, but does not require a complete list or inclusion of the first row group (all Parquet row-group offsets are only given as an example). I reproduced this with a real four-row-group file by supplying the valid sorted suffix `offsets[1:]`: `ReadTasks` returned 6 rows instead of 8. Because the first generated task starts at `offsets[0]`, no task owns an omitted leading row group. Please make the first generated range start at byte 0 (or otherwise prove completeness from the footer), and add regressions for omitted leading and interior offsets. ########## table/scan_splits.go: ########## @@ -0,0 +1,70 @@ +// 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 table + +import "github.com/apache/iceberg-go" + +// splitParquetScanTask returns one task per valid Parquet split offset when a +// complete, large-file task can be split safely. The boolean is false when the +// original task should be used unchanged. Split offsets are absolute byte +// positions in the data file and each resulting range ends at the next offset, +// or at the end of the file. +// +// Tasks that are already partial, small, non-Parquet, or missing usable split +// offsets stay unchanged. Keeping the original task in those cases preserves +// the existing behavior for remote tasks and older files. +func splitParquetScanTask(task FileScanTask, targetSize int64) ([]FileScanTask, bool) { + file := task.File + if file == nil || file.FileFormat() != iceberg.ParquetFile || targetSize <= 0 { + return nil, false + } + + fileSize := file.FileSizeBytes() + if fileSize <= targetSize || task.Start != 0 || task.Length != fileSize { Review Comment: **[P2] `read.split.target-size` is being used only as an enablement threshold, not as a target.** Once a file is one byte over the configured value, this function emits one task per recorded offset regardless of each task’s size. A file just above 128 MiB with thousands of small row groups can therefore create thousands of independently opened tasks. Please coalesce adjacent row-group ranges toward the configured target (allowing an indivisible row group to exceed it), or use and document a different threshold property rather than the standard target-size key. ########## table/internal/parquet_files.go: ########## @@ -1853,6 +1883,9 @@ func (w wrapPqArrowReader) GetRecords(ctx context.Context, cols []int, tester an *rowGroupTester.Survivors = (*rowGroupTester.Survivors)[:0] } + rangeStart := rowGroupTester.Start + rangeEnd := rangeStart + rowGroupTester.Length Review Comment: **[P2] Validate partial-task ranges before adding them.** `Start + Length` can overflow, and zero or negative lengths bypass range selection in the caller and can read the whole file rather than fail. Please reject malformed ranges using subtraction-safe checks such as `Start >= 0`, `Length > 0`, `Start <= fileSize`, and `Length <= fileSize-Start`, returning `iceberg.ErrInvalidArgument` instead of silently selecting all or no row groups. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
