Skip to content

Conversation

@sgulseth
Copy link
Member

@sgulseth sgulseth commented Sep 22, 2025

Description

Adds a temporary step when parsing functions. We first gather all the function calls and parses them as InlineFunctionCall, when we have gathered all the calls we replace them.
This is to support custom functions, where order of calls doesn't matter, so we first need to know all function definitions before we can replace them.
See #309 for more context on custom function implentation.

Most of the changes are due to changing the expression builder method

What to review

I introduced a walk method to walk the AST, however this might be slow on very large queries. Is this an issue? 🤔 Another approach would be to store a reference(or similar) to each InlineFunctionCall, and then only replace the node's values.

Testing

Did not add any tests, as this doesn't change any behavior and only refactors to be compatible with custom groq functions

@sgulseth
Copy link
Member Author

sgulseth commented Sep 22, 2025

@sgulseth sgulseth force-pushed the feat/parser-inline-func branch 2 times, most recently from 150e1ea to ea32bd5 Compare September 22, 2025 11:28
@sgulseth sgulseth changed the title feat: add inline function step to replace functions after parsing refactor: add inline function step to replace functions after parsing Sep 22, 2025
@sgulseth sgulseth requested a review from judofyr September 22, 2025 11:28
@sgulseth sgulseth marked this pull request as ready for review September 22, 2025 12:43
@sgulseth sgulseth force-pushed the feat/parser-inline-func branch from ea32bd5 to 1af2bab Compare October 13, 2025 13:53
| FilterNode
| FlatMapNode
| FuncCallNode
| InlineFuncCallNode
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some alternatives:

(1) Just remove the func definition during parsing and rather do it during execution:

export interface FuncCallNode extends BaseNode {
  type: 'FuncCall'
  namespace: string
  name: string
  args: ExprNode[]
}

(2) Make it optional:

export interface FuncCallNode extends BaseNode {
  type: 'FuncCall'
  func?: GroqFunction
  namespace: string
  name: string
  args: ExprNode[]
}

It's sort-of an optimization that we detect the function at parse time. It's probably not really worth it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't that be a breaking change though?

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.

2 participants