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.

Sign up for GitHub

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

Jump to bottom

refactor(risingwave): port to sqlglot #8171

Merged
merged 50 commits into from
Feb 1, 2024
Merged

refactor(risingwave): port to sqlglot #8171

merged 50 commits into from
Feb 1, 2024

Conversation

gforsyth
Copy link
Member

xref #7909

a lot of new passing tests -- some flakiness in groupconcat when it's used in a window.

failing tests are all assertion errors in test_array.

@cpcloud cpcloud added this to the 9.0 milestone Jan 31, 2024
@cpcloud cpcloud added the risingwave The RisingWave backend label Jan 31, 2024
@github-actions GitHub Actions
Copy link
Contributor

ACTION NEEDED

Ibis follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message.

Please update your PR title and description to match the specification.

@cpcloud cpcloud mentioned this pull request Feb 1, 2024
Closed
KeXiangWang and others added 12 commits February 1, 2024 08:01
@KeXiangWang @cpcloud
feat(risingwave): init impl for Risingwave (ibis-project#7954)
98e1173 
This is the initial PR for ibis to support risingwave.

[RisingWave](https://github.com/risingwavelabs/risingwave) is a
distributed SQL streaming database engineered to provide the simplest
and most cost-efficient approach for processing and managing streaming
data with utmost reliability.

After a few weeks of investigation, here's my phasic results.

1. As Risingwave is largely compatible with Postgres, Ibis can be easily
extended to support Risingwave. The main work is to add a new dialect
for Risingwave, which is similar to the `postgres` dialect. I've almost
finished this part in this PR. With this PR, Ibis can be used to connect
to Risingwave and run some basic queries. I've manually tested some
queries which works well and add some tests imitating Postgres Backend.
I would appreciate if you can help test more queries.

2. As ibis relies on SQLalchemy to support Postgres, we follow its
implementation to support Risingwave. However, there are also some
differences in semantics between Risingwave and Postgres, which require
some modification in either ibis or SQLalchemy. `sqlalchemy-risingwave`
is designed to reduce this mismatch. So in this PR, I introduce the new
dependencies `sqlalchemy-risingwave` to ibis.

3. Ibis has no support for Materialized View natively. However
Materialized View is a core concept in RW, people use RW because of its
convenient auto-updated Materialized View. Now, if a user wants to
create a new MV, he needs to use a raw SQL. Adding DDLs like
`CreateMaterializedView`, `CreateSource` and `CreateSink` in the [base
ddl
file](https://github.com/ibis-project/ibis/blob/main/ibis/backends/base/sql/ddl.py)
may help. We would appreciate it if you can help offer some suggestions.

Besides this, I also met some obstacle that may need your help.

1. Risingwave hasn't supported `TEMPORARY VIEW` yet, so I changed some
implementations relying on `TEMPORARY VIEW` to a normal view. For
example, for the `_metadata()` functions, RW backend's implementation is
`con.exec_driver_sql(f"CREATE VIEW IF NOT EXISTS {name} AS {query}")`.
While in PG it's `con.exec_driver_sql(f"CREATE TEMPORARY VIEW {name} AS
{query}")`. Do you have any suggestions for other ways to work around
this?
Besides, I didn't quite get what `_metadata()` is doing here. I would
appreciate it if you could explain it a bit.

2. There's some mismatch between the `postgres` dialect and `risingwave`
dialect, which are still not fully tested in this PR. We'll continue to
work on it.

3. ~~This PR requires some new features of Risingwave v1.6.0 and
sqlalchemy-risingwave v1.0.0 which are not released yet. They'll be
released soon.~~ Done. BTW, how should I indicate this backend is only
for risingwave > 1.6?

4. I don't quite understand the test pipeline of Ibis. I copied the test
cases from the `postgres` dialect and modified them to fit the
`risingwave` dialect, and some of them are commented temporarily due to
the lack of support. I also added an SQL script to help set up the test
environment, which creates tables and loads data. But I don't know how
to run it in the test pipeline. Any suggestions or guidance are
welcomed. I suppose the test pipeline would require a docker image of
Risingwave. We can provide one if needed.

5. I'm a newbie in the ibis community, this PR may not be perfect
considering others. Any comments are welcomed and I sincerely appreciate
your time and patience.

closes ibis-project#8038
@cpcloud
build(docker): simplify risingwave docker setup (ibis-project#8126)
69060cd 
Remove risingwave-specific minio service in favor of existing minio
service.
@gforsyth @cpcloud
test(risingwave): add notimpl markers for new hexdigest tests (ibis-p…
43e2f91 
…roject#8140)

Fix the failing tests on main
@gforsyth @cpcloud
fix(docker): get risingwave + minio to start up
f846d9e
@gforsyth @cpcloud
test(risingwave): testconf setup
9565227
@gforsyth @cpcloud
refactor(postgres,risingwave): separate name and dialect
d2a9b29
@gforsyth @cpcloud
fix(risingwave): test_api
cd2c1d0
@gforsyth @cpcloud
test(risingwave): fix up test_array (DANGER)
012a62d 
a bunch of tests that previously failed with a `ValueError` now return
results but the results are incorrect.  This seems bad.
@gforsyth @cpcloud
test(risingwave): xfail asof join
6c18f5b
@gforsyth @cpcloud
test(risingwave): fix up test_client.py
179655d
@gforsyth @cpcloud
feat(risingwave): add sparse risingwave dialect
d75a816
@gforsyth @cpcloud
test(risingwave): test_generic.py
77d4cd5
@cpcloud
Copy link
Member

cpcloud commented Feb 1, 2024

Looking into the rebase now...

@gforsyth
Copy link
Member Author

gforsyth commented Feb 1, 2024

I have a fix for the Trino backend incoming.

oops, I don't think I stomped on this. We definitely just fixed a bunch of the same stuff. That'll teach me to ignore my email

@kszucs kszucs force-pushed the the-epic-split branch 2 times, most recently from 3376022 to 432d151 Compare February 1, 2024 17:39
@cpcloud
Copy link
Member

cpcloud commented Feb 1, 2024

Nah, you're good. I had already pushed the fix in a previous commit!

@gforsyth gforsyth marked this pull request as ready for review February 1, 2024 18:21
cpcloud
cpcloud approved these changes Feb 1, 2024
View reviewed changes
Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

@cpcloud cpcloud merged commit 18c8cb7 into ibis-project:the-epic-split Feb 1, 2024
58 checks passed
@gforsyth gforsyth deleted the risingwave_sqlglot branch February 1, 2024 19:30
gforsyth added a commit to kszucs/ibis that referenced this pull request Feb 1, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist
refactor(risingwave): port to sqlglot (ibis-project#8171)
b44ecb1 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 2, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist @kszucs
refactor(risingwave): port to sqlglot (ibis-project#8171)
57bb4a8 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 2, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist @kszucs
refactor(risingwave): port to sqlglot (ibis-project#8171)
954c7d2 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 2, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist @kszucs
refactor(risingwave): port to sqlglot (ibis-project#8171)
954228f 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
@cpcloud cpcloud mentioned this pull request Feb 2, 2024
Closed
21 tasks
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 4, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist
refactor(risingwave): port to sqlglot (ibis-project#8171)
0d972b7 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 5, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist
refactor(risingwave): port to sqlglot (ibis-project#8171)
126f016 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
kszucs pushed a commit that referenced this pull request Feb 5, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist @kszucs
refactor(risingwave): port to sqlglot (#8171)
ba5825f 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
kszucs pushed a commit that referenced this pull request Feb 6, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist @kszucs
refactor(risingwave): port to sqlglot (#8171)
2459fdb 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
kszucs pushed a commit that referenced this pull request Feb 6, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist @kszucs
refactor(risingwave): port to sqlglot (#8171)
9ed17fc 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 12, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist
refactor(risingwave): port to sqlglot (ibis-project#8171)
20c5602 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
cpcloud added a commit that referenced this pull request Feb 12, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist
refactor(risingwave): port to sqlglot (#8171)
d54aacb 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
cpcloud added a commit to cpcloud/ibis that referenced this pull request Feb 12, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist
refactor(risingwave): port to sqlglot (ibis-project#8171)
b89c06d 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
cpcloud added a commit that referenced this pull request Feb 12, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist
refactor(risingwave): port to sqlglot (#8171)
23d3e55 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
kszucs pushed a commit that referenced this pull request Feb 12, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist @kszucs
refactor(risingwave): port to sqlglot (#8171)
1023e56 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
ncclementi pushed a commit to ncclementi/ibis that referenced this pull request Feb 21, 2024
@gforsyth @KeXiangWang
@cpcloud @jcrist @ncclementi
refactor(risingwave): port to sqlglot (ibis-project#8171)
142a597 
Co-authored-by: Kexiang Wang <[email protected]>
Co-authored-by: Phillip Cloud <[email protected]>
Co-authored-by: Jim Crist-Harif <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpcloud cpcloud cpcloud approved these changes

Assignees
No one assigned
Labels
risingwave The RisingWave backend tes-required-for-merge Issues that must addressed before merging the-epic-split branch into main
Projects
None yet
Milestone
9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@gforsyth @cpcloud @KeXiangWang