Skip to content

Conversation

@msiomkin
Copy link
Contributor

@msiomkin msiomkin commented Oct 9, 2025

Some functions may require schema before the name. E.g.: MonetDB json.isarray function: json.isarray('[1, 2, 3]'). PostgreSQL also allows it: "Schemas also contain other kinds of named objects, including data types, functions, and operators." (https://www.postgresql.org/docs/current/ddl-schemas.html). Let's support it.

Some functions may require schema before the name. E.g.: MonetDB json.isarray
function: json.isarray('[1, 2, 3]'). PostgreSQL also allows it: "Schemas also
contain other kinds of named objects, including data types, functions, and
operators." (https://www.postgresql.org/docs/current/ddl-schemas.html). Let's
support it.
@msiomkin msiomkin force-pushed the fn-schema branch 4 times, most recently from d335d43 to df8573d Compare October 9, 2025 21:17
Copy link
Member

@dey4ss dey4ss left a comment

Choose a reason for hiding this comment

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

I just have one comment. Apart from that, it looks good to go.


static Expr* makeStar(char* table);

static Expr* makeFunctionRef(char* func_name, std::vector<Expr*>* exprList, bool distinct, WindowDescription* window);
Copy link
Member

Choose a reason for hiding this comment

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

This method is not really required anymore, right? We could just use the new one and pass nullptr if there is no schema defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can if you insist on it. But I worry about backward compatibility. We don't need schema on functions very often. Most users use this method without schema. They'll have to fix their calls. More than that, they'll have to pass nullptr explicitly for a parameter they don't need 90% of time.

Copy link
Member

Choose a reason for hiding this comment

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

That's a valid point. Then, fine for me to go. Thanks for your contribution!

@dey4ss dey4ss merged commit ccd3f68 into hyrise:main Oct 30, 2025
4 checks passed
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