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 chsql extension #43

Merged
merged 2 commits into from
Jul 9, 2024
Merged

Add chsql extension #43

merged 2 commits into from
Jul 9, 2024

Conversation

lmangani
Copy link
Contributor

@lmangani lmangani commented Jul 9, 2024

Adding chsql extension to Community 🤞

This extension provides a growing number of Clickhouse SQL Macros for DuckDB.

Special thanks @carlopi for all the support and assistance backporting the builders to v1.0.0 ⭐

@lmangani lmangani marked this pull request as ready for review July 9, 2024 17:26
Copy link
Collaborator

@carlopi carlopi left a comment

Choose a reason for hiding this comment

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

I gave a brief looks, we would need to write a proper template for SQL-only extensions with @Alex-Monahan, but that's for later.

I noticed in the macros there is a:

CREATE OR REPLACE MACRO intDiv(a, b) AS (a / b);
CREATE OR REPLACE MACRO intDivOrZero(a, b) AS COALESCE((a / b), 0);

but // should be used for integer division, otherwise you will get a floating point number as result.

Thanks, and Welcome!

extensions/chsql/description.yml Outdated Show resolved Hide resolved
@carlopi carlopi merged commit ddc50a3 into duckdb:main Jul 9, 2024
2 checks passed
@lmangani
Copy link
Contributor Author

lmangani commented Jul 9, 2024

I gave a brief looks, we would need to write a proper template for SQL-only extensions with @Alex-Monahan, but that's for later.

Is anything out of place? I'd be curious to learn and improve and/or act as a guinea duck anytime

CREATE OR REPLACE MACRO intDiv(a, b) AS (a / b);

but // should be used for integer division, otherwise you will get a floating point number as result.

We don't actually use the aliases sql file, its purely a reference for testing and understanding the logic.

The actual code is as follows:

{DEFAULT_SCHEMA, "intDiv", {"a", "b"}, R"((CAST(a AS BIGINT) / CAST(b AS BIGINT)))"},

Does it look proper?

@lmangani
Copy link
Contributor Author

lmangani commented Jul 9, 2024

Exciting and works out of the box 😂
image

@carlopi
Copy link
Collaborator

carlopi commented Jul 10, 2024

I think you still need a double //:

D INSTALL chsql FROM community;
D LOAD chsql;
D SELECT intDiv(5,2);
┌──────────────┐
│ intdiv(5, 2) │
│    double    │
├──────────────┤
│          2.5 │
└──────────────┘

see also https://duckdb.org/docs/sql/functions/numeric.html#division-and-modulo-operators

D SELECT 5/2;
┌─────────┐
│ (5 / 2) │
│ double  │
├─────────┤
│     2.5 │
└─────────┘
D SELECT 5//2;
┌──────────┐
│ (5 // 2) │
│  int32   │
├──────────┤
│        2 │
└──────────┘

@lmangani
Copy link
Contributor Author

@carlopi totally missed that and wrongly assumed you were referring to casting.
Great input! I will update the macro and experiment with our first extension upgrade :)

lmangani added a commit to quackscience/duckdb-extension-clickhouse-sql that referenced this pull request Jul 10, 2024
Essential fix based on [thread suggestion](duckdb/community-extensions#43 (review))
@lmangani
Copy link
Contributor Author

Fixed in 17c249ba08a9b88338e77c7f2d6e5dd2040b4590
@carlopi should I just repoint the extension ref or is a minor version increase required to cycle?

@carlopi
Copy link
Collaborator

carlopi commented Jul 10, 2024

Truth is that version field is, at the moment, not really used, only relevant information, also used on extension install is the ref.
In the run-up to 1.1 we will iterate and specify more precisely the meaning of the version.

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