feat: implement `unwrap` as a projection by trevorwhitney · Pull Request #19409 · grafana/loki

trevorwhitney · 2025-10-07T20:32:08Z

What this PR does / why we need it:

This PR continues our efforts towards LogQL backwards compatibility, with a focus on queries needed for Drilldown, by implementing support for unwrap. Unwrap was implemented as a projection.

Special notes for your reviewer:

Checklist

Reviewed the CONTRIBUTING.md guide (required)
Documentation added
Tests updated
Title matches the required conventional commits format, see here
- Note that Promtail is considered to be feature complete, and future development for logs collection will be in Grafana Alloy. As such, feat PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.
Changes that require user attention or interaction to upgrade are documented in docs/sources/setup/upgrade/_index.md
If the change is deprecating or removing a configuration option, update the deprecated-config.yaml and deleted-config.yaml files respectively in the tools/deprecated-config-checker directory. Example PR

chaudum · 2025-10-16T07:45:11Z

+	UnaryOpUnwrapBytes    // Unwrap string bytes to float value operation (unwrap).
+	UnaryOpUnwrapDuration // Unwrap string duration to float value operation (unwrap).


I don't think we should have individual operations for unwrap bytes and unwrap duration.

without the different types, how do we determine the correct conversion function? would you parse that during logical planning and pass that information through in a way other than the operation? the pattern for registering functions leaned heavily on operation types, so it seemed like a good place to make the differentiation to me.

@chaudum and I agreed offline we'll need different operations, but they should be renamed to cast operations as that's what these actually represent, ie. how to do the cast.

chaudum · 2025-10-16T07:46:36Z

+const (
+	// UnwrapOpInvalid indicates an invalid unwrap operation.
+	UnwrapOpInvalid UnwrapOp = iota
+	Unwrap


Is this considered an unwrap INT or unwrap FLOAT, or generic unwrap NUMBER?

Unwrap would be a unwrap FLOAT -> strconv.ParseFloat(v, 64)

renamed to cast and got made the base case more explicit by naming it CastFloat

rfratto

Nice work! I really like how this flows as a projection and unary expression.

rfratto · 2025-10-17T18:27:21Z

+	UnaryOpCastFloat    // Cast string to float value operation (unwrap).
+	UnaryOpCastBytes    // Cast string bytes to float value operation (unwrap).
+	UnaryOpCastDuration // Cast string duration to float value operation (unwrap).


all of these say "to float value", but is that correct?

I think so, because the type of the value column the casted result is put in will always be a float64, and the conversion functions all return a float64. The difference is in how they get that value (ie. strconv, time.Parse, or humanize)

rfratto · 2025-10-17T18:29:59Z

+// Unwrap applies an [Projection] operation, with an [UnaryOp] unwrap operation, to the Builder.
+func (b *Builder) Unwrap(identifier string, operation string) *Builder {


Hm, I intended Builder to have an API which more closely reflects the logical plan rather than LogQL constructs.

Can we update this to Builder.Cast maybe?

oh, interesting, I though of builder as the bridge between logql and the logical plan, so the parameters of this function match closely to what we get out of logql for the unwrap operation. I'm happy to go with Cast though, as I think the arguments are still consistent.

rfratto · 2025-10-17T18:32:48Z

+			// Check for unwrap in the LogRangeExpr
+			if e.Left.Unwrap != nil {
+				unwrapIdentifier = e.Left.Unwrap.Identifier
+				unwrapOperation = e.Left.Unwrap.Operation
+			}


Is it valid for a LogQL query to have more than one unwrap? If so, this breaks.

Maybe in the meantime, we can return errUnimplemented if we see an unwrap but unwrapIdentifier is already set?

I can return errUnimplemented 👍 . The only way I can think of having multiple unwrap statements is on opposite sides of a binary operation, so in two separate range expressions. I haven't thought about how that would be handled in this PR.

rfratto · 2025-10-17T18:33:39Z

+			// Check for unwrap in the LogRangeExpr
+			if e.Left.Unwrap != nil {
+				unwrapIdentifier = e.Left.Unwrap.Identifier
+				unwrapOperation = e.Left.Unwrap.Operation


IMO we should be validating that we recognize and support whatever e.Left.Unwrap.Operation is and return errUnimplmeneted otherwise.

rfratto · 2025-10-17T18:43:59Z

+			}
+		}
+
+		schema := arrow.NewSchema(fields, nil)


Should this propagate the metadata from the input? Are we using metadata anywhere anymore? (Ditto with newKeepPipeline above)

rfratto · 2025-10-17T18:44:35Z

 }
+
+func newExpandPipeline(expressions []physical.UnaryExpression, evaluator *expressionEvaluator, input Pipeline) (*GenericPipeline, error) {
+	return newGenericPipeline(func(ctx context.Context, inputs []Pipeline) (arrow.Record, error) {


I think ideally we validate that len(inputs) == 1 here so things don't silently break. Ditto with newKeepPipeline

rfratto · 2025-10-17T18:46:08Z

-		pipeline := NewFilterPipeline(filter, input, expressionEvaluator{}, alloc)
-		defer pipeline.Close()
+		e := newExpressionEvaluator(alloc)
+		pipeline := NewFilterPipeline(filter, input, e, alloc)


I think we're missing a defer pipeline.Close() here, it was there but got removed in the diff

ahh, good catch. many rebases

rfratto · 2025-10-17T18:59:12Z

+		if errTracker.hasErrors {
+			defer errorCol.Release()
+			defer errorDetailsCol.Release()
+		}


You can remove this since that's already handled by the defer to releaseBuilders on the line above

rfratto · 2025-10-17T19:00:14Z

+func buildOutputFields(
+	hasErrors bool,
+) []arrow.Field {
+	fields := make([]arrow.Field, 0, 3)
+
+	// Add value field. Not nullable in practice since we use 0.0 when conversion fails, but as of
+	// writing all coumns are marked as nullable, even Timestamp and Message, so staying consistent
+	fields = append(fields, semconv.FieldFromIdent(semconv.ColumnIdentValue, true))
+
+	// Add error fields if needed
+	if hasErrors {
+		fields = append(fields,
+			semconv.FieldFromIdent(semconv.ColumnIdentError, true),
+			semconv.FieldFromIdent(semconv.ColumnIdentErrorDetails, true),
+		)
+	}
+
+	return fields
+}
+
+func buildResult(


buildOutputFields and buildResult might be a little too generically named for being in the executorPackage, can we give them more precise names?

chaudum

Nothing more to add than what has already been said.

trevorwhitney requested a review from a team as a code owner October 7, 2025 20:32

pull-request-size Bot added the size/XXL label Oct 7, 2025

trevorwhitney force-pushed the twhitney/thor-unwrap branch from de9b26a to 9007cf8 Compare October 7, 2025 20:45

trevorwhitney commented Oct 7, 2025

View reviewed changes

Comment thread pkg/engine/internal/executor/expressions.go

trevorwhitney commented Oct 7, 2025

View reviewed changes

Comment thread pkg/engine/internal/executor/project.go Outdated

trevorwhitney commented Oct 7, 2025

View reviewed changes

Comment thread pkg/engine/internal/types/column.go

trevorwhitney force-pushed the twhitney/thor-unwrap branch 2 times, most recently from ba88390 to 0f7c19b Compare October 8, 2025 22:05

rfratto reviewed Oct 9, 2025

View reviewed changes

chaudum reviewed Oct 16, 2025

View reviewed changes

trevorwhitney requested review from JoaoBraveCoding, periklis and xperimental as code owners October 16, 2025 16:24

trevorwhitney force-pushed the twhitney/thor-unwrap branch from 5e75771 to 4ec6ce1 Compare October 16, 2025 16:45

ivkalita mentioned this pull request Oct 17, 2025

chore(engine): remove unused logical.Value.Schema() #19531

Merged

6 tasks

chaudum reviewed Oct 17, 2025

View reviewed changes

Comment thread pkg/engine/internal/executor/cast.go Outdated

Comment thread pkg/engine/internal/executor/cast.go Outdated

Comment thread pkg/engine/internal/executor/executor.go Outdated

Comment thread pkg/engine/internal/planner/logical/planner_test.go Outdated

trevorwhitney added 2 commits October 17, 2025 10:42

feat: implement unwrap as projection with unary expr

d40706d

chore: cleanup from rebase/merge

e2f6ebf

trevorwhitney force-pushed the twhitney/thor-unwrap branch from 894b194 to e2f6ebf Compare October 17, 2025 16:57

trevorwhitney added 4 commits October 17, 2025 11:20

chore: more merge cleanup, add planner test

b8f40cb

chore: a bit more cleanup and PR feedback

0ecae98

test: fixe expected error message

71190b8

Merge branch 'main' into twhitney/thor-unwrap

2e0b5ec

rfratto approved these changes Oct 17, 2025

View reviewed changes

chaudum approved these changes Oct 17, 2025

View reviewed changes

trevorwhitney added 3 commits October 17, 2025 14:04

chore: small changes from PR review

16ce808

chore: format

0b82d14

Merge branch 'main' into twhitney/thor-unwrap

cd90e60

trevorwhitney merged commit 3ce6fa2 into main Oct 17, 2025
63 checks passed

trevorwhitney deleted the twhitney/thor-unwrap branch October 17, 2025 20:27

chaudum mentioned this pull request Oct 22, 2025

feat(engine): basic math expressions #19407

Merged

6 tasks

loki-gh-app Bot mentioned this pull request Apr 2, 2026

chore(release-3.6.x): release 3.6.10 #21380

Merged

This was referenced Jun 23, 2026

chore(release-3.6.x): Release 3.6.0 #22541

Closed

chore(release-3.6.x): Release 3.6.12 #22588

Merged

		UnaryOpUnwrapBytes // Unwrap string bytes to float value operation (unwrap).
		UnaryOpUnwrapDuration // Unwrap string duration to float value operation (unwrap).

		// Unwrap applies an [Projection] operation, with an [UnaryOp] unwrap operation, to the Builder.
		func (b Builder) Unwrap(identifier string, operation string) Builder {

Uh oh!

Conversation

trevorwhitney commented Oct 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rfratto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chaudum left a comment

Choose a reason for hiding this comment

Uh oh!

Labels

5 participants