Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise error on non-resolution of function #576

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jikrow
Copy link

@jikrow jikrow commented Dec 30, 2024

This PR fixes #573

@jikrow
Copy link
Author

jikrow commented Dec 30, 2024

test-sqlite is failing since sqlite's error message is slightly different. @jussisaurio, this is perhaps also related to your comment in #567, where you question whether to bind functions in planner or during bytecode emmision.
I think it's best to add this test case at a later stage when we are closer to sqlite's implementation of binding symbols. Let me know what you think!

@jussisaurio
Copy link
Collaborator

jussisaurio commented Dec 31, 2024

There's two paths to take here:

  1. if we want to bind functions in planner, then this is OK, however then External functions #567 needs to follow suit accordingly.

  2. if we do the binding ad-hoc in the bytecode emission phase per function invocation, then the best way forward here is to treat unresolved functions the same way as resolved ones:

                                    Err(_) => {
                                        // Ignore unresolved function, it will be resolved to an external function later, or a parse error will be returned
                                        let contains_aggregates =
                                            resolve_aggregates(&expr, &mut aggregate_expressions);
                                        plan.result_columns.push(ResultSetColumn {
                                            expr: expr.clone(),
                                            contains_aggregates,
                                        });
                                    }

The biggest problem with 2, i.e. not binding in the planning stage, is that then we have no idea whether the extension-defined function (which presents itself as "any unrecognized function" to the planner) is itself an aggregate function. This will result in incorrect behavior, since we never invoke aggregate functions inside the main query loop: for example, we don't call SUM(x) for every row of the loop -- we evaluate x for every row and then call SUM after the loop:

https://github.com/tursodatabase/limbo/blob/main/core/translate/emitter.rs#L1057-L1071
https://github.com/tursodatabase/limbo/blob/main/core/translate/expr.rs#L2147

cc @penberg

@jussisaurio jussisaurio mentioned this pull request Dec 31, 2024
@jussisaurio
Copy link
Collaborator

We decided to merge #567 as is, so this PR should be changed to ignore the error from resolving the function - effectively do option 2 above.

We will cross the bridge of handling externally defined aggregate functions when that becomes relevant.

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.

nonexistent functions in select clause result columns are ignored
2 participants