Skip to content

Conversation

@owen-d
Copy link
Contributor

@owen-d owen-d commented Apr 18, 2025

Dependent on / followup to #17311

Implement Projection Pipeline in Engine Executor

Changes

This PR adds projection support to the Loki query engine executor, allowing queries to select specific columns from record batches. Key changes include:

  • Add NewProjectPipeline function for creating projection pipelines
  • Implement the previously unimplemented executeProjection method
  • Add evaluator field to the Context struct for expression evaluation
  • Improve CSV to Arrow conversion by trimming whitespace
  • Add comprehensive test cases for projections

Test Coverage

The implementation includes thorough tests that verify:

  • Single column projection
  • Multiple column projection
  • Column reordering
  • Processing multiple input batches

@owen-d owen-d requested a review from a team as a code owner April 18, 2025 00:25
return failureState(err)
}

batch, err := input.Value()
Copy link
Contributor

Choose a reason for hiding this comment

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

do not have a strong opinion about this. but with this API design, the same err is returned twice once with input.Read() and then again with input.Value()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain a bit more? The first error comes from the child Pipeline (via Read()) and the second from the same pipeline via Value(). I don't think we can make assumptions about these being the same, although it's possible we could revisit the Pipeline interface, but that's beyond the scope of this PR

Copy link
Contributor

@ashwanthgoli ashwanthgoli left a comment

Choose a reason for hiding this comment

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

lgtm

@owen-d owen-d force-pushed the dataobj-executor-project-node branch from d51ea72 to 9f5f50c Compare April 18, 2025 15:47
@owen-d owen-d merged commit 395030b into grafana:main Apr 18, 2025
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants