Repository navigation
importinto: log chunk count and size - #70271
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
release-note-none
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe importer now reports subtask chunk counts and source chunk sizes in structured logs. ChangesImport observability
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/executor/importer/table_import.go (1)
122-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument both
GetSizesemantics.
GetSizereturnsEndOffset - Offsetfor non-Parquet chunks butFileSizefor Parquet chunks. Update the exported comment to describe both branches so callers do not interpretchunkSizeas a byte span for Parquet.As per coding guidelines, exported-symbol comments should preserve semantic constraints.
Proposed comment
-// GetSize get the original file size of this chunk. +// GetSize returns the source size represented by the chunk. +// For Parquet chunks, Offset and EndOffset are row offsets, so FileSize is returned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/importer/table_import.go` around lines 122 - 129, Update the exported comment for Chunk.GetSize to document both semantics: non-Parquet chunks return the byte span EndOffset minus Offset, while Parquet chunks return FileSize because Offset and EndOffset represent rows.Source: Coding guidelines
pkg/executor/importer/chunk_process_testkit_test.go (1)
147-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise offset subtraction in the test.
chunkInfo.Offsetis zero, solen(sourceData)equalsEndOffset. The test would pass ifGetSizeignoredOffset. Use a valid non-zero row-boundary offset or add focused tests for both non-Parquet and Parquet chunks.The PR objective requires the logged size to reflect the chunk-size contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/importer/chunk_process_testkit_test.go` around lines 147 - 160, Update the test around NewFileChunkProcessor to exercise the chunk-size contract with a non-zero chunkInfo.Offset, setting EndOffset so the expected logged chunkSize equals EndOffset minus Offset. Ensure the assertions validate that subtraction rather than merely comparing against len(sourceData), and add equivalent focused coverage for both non-Parquet and Parquet chunk paths if they are separate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/executor/importer/chunk_process_testkit_test.go`:
- Around line 147-160: Update the test around NewFileChunkProcessor to exercise
the chunk-size contract with a non-zero chunkInfo.Offset, setting EndOffset so
the expected logged chunkSize equals EndOffset minus Offset. Ensure the
assertions validate that subtraction rather than merely comparing against
len(sourceData), and add equivalent focused coverage for both non-Parquet and
Parquet chunk paths if they are separate.
In `@pkg/executor/importer/table_import.go`:
- Around line 122-129: Update the exported comment for Chunk.GetSize to document
both semantics: non-Parquet chunks return the byte span EndOffset minus Offset,
while Parquet chunks return FileSize because Offset and EndOffset represent
rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d7dc4f44-8e38-4b39-bd2e-2207f4ed53d8
📒 Files selected for processing (5)
pkg/dxf/importinto/task_executor.gopkg/executor/importer/BUILD.bazelpkg/executor/importer/chunk_process.gopkg/executor/importer/chunk_process_testkit_test.gopkg/executor/importer/table_import.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #70271 +/- ##
================================================
- Coverage 76.3269% 73.5652% -2.7617%
================================================
Files 2041 2078 +37
Lines 559033 582628 +23595
================================================
+ Hits 426693 428612 +1919
- Misses 131440 153559 +22119
+ Partials 900 457 -443
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
|
🔍 Starting code review for this PR... |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 2
- Inline comments: 1
- Summary-only findings (no inline anchor): 1
Findings (highest risk first)
🟡 [Minor] (2)
- chunkSize log field only added to the file-based chunk processor, not the parallel query-based one (pkg/executor/importer/chunk_process.go:444, pkg/executor/importer/chunk_process.go:592)
- GetSize doc comment says "original file size" but returns a per-chunk byte range for non-parquet sources (pkg/executor/importer/table_import.go:122, pkg/executor/importer/table_import.go:106)
Unanchored findings
🟡 [Minor] (1)
- chunkSize log field only added to the file-based chunk processor, not the parallel query-based one
- Request: Either add an equivalent size-related field to
newQueryChunkProcessor's logger (even if it is a query-specific proxy such as row count, or explicitly omitted with a comment noting query chunks have no byte size), or keep the two constructors' logger enrichment intentionally documented as diverging so the field isn't assumed universal.
- Request: Either add an equivalent size-related field to
| return c.Path + ":" + strconv.FormatInt(c.Offset, 10) | ||
| } | ||
|
|
||
| // GetSize get the original file size of this chunk. |
There was a problem hiding this comment.
🟡 [Minor] GetSize doc comment says "original file size" but returns a per-chunk byte range for non-parquet sources
Impact
For every non-parquet chunk, GetSize returns EndOffset - Offset, which is the byte range assigned to this specific chunk, not the size of the source file it came from. The Chunk struct already has a distinct FileSize field (the actual original file size, see table_import.go:106 and its use at table_import.go:539) populated from region.FileMeta.FileSize when large files are split into multiple chunks by MakeTableRegions.
A reader who trusts the doc comment could assume GetSize() and FileSize are interchangeable, which is wrong whenever a file is split (StrictFormat/MaxChunkSize) into more than one chunk.
Scope
pkg/executor/importer/table_import.go:122—Chunk.GetSizepkg/executor/importer/table_import.go:106—Chunk.FileSize
Evidence
table_import.go:122-129 defines GetSize with the comment // GetSize get the original file size of this chunk., but the body returns c.EndOffset - c.Offset for non-parquet types — a sub-range of the file, not the file's original size. The struct already carries the true original file size in the separate FileSize field, populated from region.FileMeta.FileSize (table_import.go:539), so the two concepts (chunk byte range vs. whole-file size) are conflated under one misleading comment.
Change request
Reword the comment to describe what is actually returned, e.g. "GetSize returns the byte size of this chunk's data range (or the file size for parquet, where Offset/EndOffset are row indices), used for size-based logging/decisions" — and make clear it can be smaller than FileSize when the source file is split into multiple chunks.
|
/retest |
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GMHDBJD, joechenrh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest |
|
/cherry-pick release-nextgen-202603 |
|
@D3Hunter: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
5 participants
What problem does this PR solve?
Issue Number: ref #61702
Problem Summary:
Import Into encode subtasks do not currently expose how many chunks they need to process or the size of each chunk, making chunk-related failures harder to diagnose.
What changed and how does it work?
EndOffset - Offset, to the per-chunk logger.Check List
Tests
parquet
csv
Targeted validation:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Enhancements
Tests