Skip to content

Conversation

@wmTJc9IK0Q
Copy link

@wmTJc9IK0Q wmTJc9IK0Q commented Nov 23, 2025

Fixes #66

This moves the projection mapping logic from Row to DataChunk and handles it consistently in one place for FillRow or FillChunk in table UDFs.

@wmTJc9IK0Q wmTJc9IK0Q marked this pull request as ready for review November 24, 2025 04:09
Copilot AI review requested due to automatic review settings November 24, 2025 04:09
Copilot finished reviewing on behalf of wmTJc9IK0Q November 24, 2025 04:12
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the projection mapping logic for table UDFs by moving it from the Row structure to the DataChunk structure. This consolidates projection handling in one place, making it consistent for both row-based (FillRow) and chunk-based (FillChunk) table UDF implementations.

Key changes:

  • Moved projection mapping from Row to DataChunk for centralized handling
  • Updated Row methods to delegate projection logic to the underlying DataChunk
  • Added projection-aware logic to DataChunk's GetValue, SetValue, and SetChunkValue methods
  • Separated projection-specific tests into dedicated test functions for better coverage

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
table_udf.go Assigns projection mapping to DataChunk instead of Row for consistent projection handling across all table UDF types
row.go Removes projection field from Row struct and delegates projection logic to the underlying DataChunk
data_chunk.go Adds projection field and implements projection-aware column access with automatic remapping of column indices
errors.go Adds new error function for unprojected column access errors
table_udf_test.go Adds dedicated tests for chunk-based and row-based projection pushdown, separates pushdownTableUDF test into standalone test function

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

data_chunk.go Outdated
Comment on lines 67 to 70
if len(chunk.projection) == 0 {
if colIdx >= len(chunk.columns) {
return getError(errAPI, columnCountError(colIdx, len(chunk.columns)))
}
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing bounds check for negative colIdx values when no projection is set. The code checks colIdx >= len(chunk.columns) but not colIdx < 0, which could lead to a panic. The check should be:

if colIdx < 0 || colIdx >= len(chunk.columns) {
    return getError(errAPI, columnCountError(colIdx, len(chunk.columns)))
}

Copilot uses AI. Check for mistakes.
Copy link

@VGSML VGSML left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wmTJc9IK0Q Could you try to avoid using nested if and if else construction. It's looks like code can be more readable

@taniabogatsch taniabogatsch added feature / enhancement Code improvements or a new feature changes requested Changes have been requested to a PR or issue labels Nov 24, 2025
@wmTJc9IK0Q
Copy link
Author

@VGSML Thanks for the review, I've cleaned this up.

@wmTJc9IK0Q wmTJc9IK0Q requested a review from VGSML November 28, 2025 03:59
Copy link

@VGSML VGSML left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! @taniabogatsch could you take a look at

Copy link
Collaborator

@taniabogatsch taniabogatsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I've left some comments and a few nits. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes requested Changes have been requested to a PR or issue feature / enhancement Code improvements or a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ChunkTableSource doesn't handle projection pushdown

3 participants