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

Add SQL scalar and table macros and tests #84

Closed
wants to merge 3 commits into from

Conversation

Alex-Monahan
Copy link

@Alex-Monahan Alex-Monahan commented Sep 16, 2024

Hi folks!

This adds support for SQL scalar and table macros to the extension template.

I am super open to feedback - I added a big comment block within the .cpp file since our audience for this is SQL folks without C++ knowledge. If you would prefer to see it in a README or elsewhere, just let me know and I can move/adjust.

The goal is to publish the SQL-only extension blog post that relies on this feature by next Friday 2024-09-27 this Friday, 2024-09-20, but I don't think we have anything on the blog schedule for the following week. I am flexible!

@szarnyasg as an FYI since this is related to the upcoming blog!

As a note, this is also dependent on a 1.1.0 feature, so it is not backward compatible with 1.0.0. However, the supported versions table did not include 1.0.0, so I felt comfortable doing it this way!

@Alex-Monahan
Copy link
Author

Howdy @carlopi and @samansmink ! Would you be able to give this a review in the next week? I'd love to merge this before blogging about it, and I'm aiming for next Friday (9/27) to publish. Cheers!

Copy link
Collaborator

@samansmink samansmink left a comment

Choose a reason for hiding this comment

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

Hey Alex! Thanks for the PR! I think SQL-only extensions are a great idea, looking forward to the blogpost.

I do feel this might bloat the extension template a little bit in its current state. I feel the extension template should remain as few lines as possible. Any documentation or examples of building extensions should go elsewhere I feel. If we start adding more an more examples to it I feel we can confuse people cloning the template.

happy to discuss this though if you or @carlopi disagree

@Alex-Monahan
Copy link
Author

Alex-Monahan commented Sep 20, 2024

Ok! Mind if I make a second template just for SQL then? I can strip it way down (removing the C++ examples).

@Alex-Monahan
Copy link
Author

I also reworked this to be as few lines as possible in case it is helpful. Does this make it easier to merge? I also moved everything to the bottom (of both the .cpp file and the tests).

If I make another template (let me know if that sounds ok!), would you like for it to be one of my repos or a duckdb repo?

@Alex-Monahan
Copy link
Author

Howdy @carlopi and @samansmink - thanks for taking a look!

I'd like to publish the blog tomorrow and I think I have 2 options:

In the future, there could be an option 3:

  • Move the sql-only-duckdb-extension into the DuckDB organization

Which option should we go with? Thanks!

@carlopi
Copy link
Contributor

carlopi commented Sep 26, 2024

Hi @Alex-Monahan, this was on me, sorry for being late.

I would say moving this to sql-only-duckdb-extension, possibly moving already that today/tomorrow with the duckdb org.

I would also file that as part of the community extension, I think it does make sense to have also templates there.

I think a cool idea would also be:

  • move the macro declaration code to a header
  • have that header generated (via some python script or so) starting from a json file or some other non C++ file format

Advantages of this is saying to people: you just modify this JSON file, run the python script (that performs some checks, possibly), and that's basically it.

JSON file (or whatever the format) can also have additional fields backed in like examples / comments / what not, but then only the needed ones are used.

This could work both with C++-based extensions but later switch transparently to C-API-only extensions.

@Alex-Monahan
Copy link
Author

Sounds good! I think moving to another format could work great. The stretch goal I have is to actually allow folks to just paste in their create macro statements and then parse them into the format that C++ needs. So I think we could even just use a .sql file as the input. For another day! :-)

The sql-only-duckdb-extension repo is fixed and up to date now I believe. It looks like somehow I have the power to create a new repo within the DuckDB org.

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.

3 participants