Skip to content

feat(firestore): new generic pipeline expressions#16550

Draft
daniel-sanche wants to merge 8 commits intomainfrom
firestore_pipelines_common_expressions
Draft

feat(firestore): new generic pipeline expressions#16550
daniel-sanche wants to merge 8 commits intomainfrom
firestore_pipelines_common_expressions

Conversation

@daniel-sanche
Copy link
Copy Markdown
Contributor

@daniel-sanche daniel-sanche commented Apr 3, 2026

WIP

New Expressions:

  • offset
  • nor
  • coalesce
  • switch_on
  • parent
  • array_filter
  • array_transform
  • storage_size
  • reference_slice

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several new pipeline expressions to the Firestore library, including offset, coalesce, switch_on, parent, and a logical Nor operator, supported by new system and unit tests. The reviewer suggested refactoring the coalesce and switch_on methods for better conciseness and identified typos in the switch_on docstring and a unit test variable name.

Comment on lines +1052 to +1056
args = [self]
args.extend(
[Expression._cast_to_expr_or_convert_to_constant(x) for x in others]
)
return FunctionExpression("coalesce", args)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

For improved readability and conciseness, you can construct the arguments list and return in a single statement.

return FunctionExpression(
    "coalesce",
    [self] + [Expression._cast_to_expr_or_convert_to_constant(x) for x in others],
)

*others: Additional alternating conditions and results, optionally followed by a default value.

Returns:
An Expression representing the switchOn operation.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

There's a small inconsistency in the docstring. The operation name is switch_on, but it's written as switchOn here. For consistency with the function name and implementation, it should be switch_on.

Suggested change
An Expression representing the switchOn operation.
An Expression representing the switch_on operation.

Comment on lines +1082 to +1086
args = [self, Expression._cast_to_expr_or_convert_to_constant(result)]
args.extend(
[Expression._cast_to_expr_or_convert_to_constant(x) for x in others]
)
return FunctionExpression("switch_on", args)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Similar to coalesce, this can be simplified into a single return statement for better readability.

return FunctionExpression(
    "switch_on",
    [self, Expression._cast_to_expr_or_convert_to_constant(result)]
    + [Expression._cast_to_expr_or_convert_to_constant(x) for x in others],
)

Comment on lines +763 to +764
infix_istance = arg1.offset(arg2)
assert infix_istance == instance
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

There's a typo in the variable name infix_istance. It should be infix_instance.

infix_instance = arg1.offset(arg2)
assert infix_instance == instance

@daniel-sanche daniel-sanche changed the title feat(firestore): new generic expressions feat(firestore): New generic pipeline expressions Apr 3, 2026
@daniel-sanche daniel-sanche changed the title feat(firestore): New generic pipeline expressions feat(firestore): new generic pipeline expressions Apr 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant