-
Notifications
You must be signed in to change notification settings - Fork 25
Add null engine table implementation #3029
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
base: main
Are you sure you want to change the base?
Add null engine table implementation #3029
Conversation
|
Someone is attempting to deploy a commit to the Fiveonefour Team on Vercel. A member of the Team first needs to authorize it. |
…s_to_python in generate.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Null engine incorrectly allows PARTITION BY clause
The supports_partition_by matching at lines 2522-2532 doesn't include ClickhouseEngine::Null, which means Null engine tables will incorrectly permit PARTITION BY clauses. This is inconsistent with the Python validation in olap_table.py (lines 207-215) which correctly rejects PARTITION BY for NullEngine. ClickHouse's Null engine doesn't support PARTITION BY, so attempting to create such tables will fail at runtime despite passing Rust-side validation.
apps/framework-cli/src/infrastructure/olap/clickhouse/queries.rs#L2521-L2532
moosestack/apps/framework-cli/src/infrastructure/olap/clickhouse/queries.rs
Lines 2521 to 2532 in 5d180ef
| let supports_sample_by = table.engine.is_merge_tree_family(); | |
| let supports_partition_by = matches!( | |
| table.engine, | |
| ClickhouseEngine::MergeTree | |
| | ClickhouseEngine::ReplacingMergeTree { .. } | |
| | ClickhouseEngine::AggregatingMergeTree | |
| | ClickhouseEngine::SummingMergeTree { .. } | |
| | ClickhouseEngine::ReplicatedMergeTree { .. } | |
| | ClickhouseEngine::ReplicatedReplacingMergeTree { .. } | |
| | ClickhouseEngine::ReplicatedAggregatingMergeTree { .. } | |
| | ClickhouseEngine::ReplicatedSummingMergeTree { .. } | |
| | ClickhouseEngine::S3 { .. } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the contribution!
Looks like this is directionality correct - you are missing the TS implementation though 🙂
examples/null-engine-example/null-engine-example-py/requirements.txt
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,26 @@ | |||
| language = "python" # Must be typescript or python | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Template definitions are in a different repo - looks like your AI mixed examples and templates.
We run templates/typescript-tests and templates/python-tests in the CI.
You could remove this example altogether and add it there for automated testing.
The prompt could be something along the lines of:
"The end-to-end test is being run by apps/framework-cli-e2e please add the null engine to the templates that are used in the tests: templates/typescript-tests and templates/python-tests"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- TS implementation
- Moved null-engine-example to template using framwork-cli-e2e
- Avoid truncate behavior on null engine table link to olap change on null engine target table of materialized view
|
Error occurs while setting null engine table as target table of materialized view because the webserver is trying to apply TRUNCATE TABLE statement which is not possible on null engine tables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Null engine missing from partition support check
The supports_partition_by check doesn't include ClickhouseEngine::Null in the match statement, which means it defaults to false correctly by the match else clause. However, the explicit enumeration pattern used for other engines should include Null for clarity and consistency with validation in the Python SDK which explicitly lists NullEngine in engines_without_partition_by. The Null engine doesn't support PARTITION BY, ORDER BY, or SAMPLE BY clauses.
apps/framework-cli/src/infrastructure/olap/clickhouse/queries.rs#L2521-L2531
moosestack/apps/framework-cli/src/infrastructure/olap/clickhouse/queries.rs
Lines 2521 to 2531 in 3dcbf7c
| let supports_sample_by = table.engine.is_merge_tree_family(); | |
| let supports_partition_by = matches!( | |
| table.engine, | |
| ClickhouseEngine::MergeTree | |
| | ClickhouseEngine::ReplacingMergeTree { .. } | |
| | ClickhouseEngine::AggregatingMergeTree | |
| | ClickhouseEngine::SummingMergeTree { .. } | |
| | ClickhouseEngine::ReplicatedMergeTree { .. } | |
| | ClickhouseEngine::ReplicatedReplacingMergeTree { .. } | |
| | ClickhouseEngine::ReplicatedAggregatingMergeTree { .. } | |
| | ClickhouseEngine::ReplicatedSummingMergeTree { .. } |
|
@slgeliberty Nice Progress! This is going in the right direction 😄 |
…ialized view target table
| ) -> bool { | ||
| if table.name == target_table { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Table matching ignores database when names match
The table_matches_mv_target function returns true when table names match without checking if databases also match. The early return at line 437-439 (if table.name == target_table { return true; }) bypasses all database checks. When a Null engine table exists with the same name as the MV target but in a different database, this incorrectly identifies it as the target. This causes target_is_null_engine to be true and should_truncate to be set to false when it should be true, potentially leading to incorrect materialized view population behavior.
|
@slgeliberty Do you want to push this accross the finish line or do you want us to help? |
Note
Adds end-to-end ClickHouse Null engine support (parsing, DDL/serialization, codegen, SDK validation), skips MV truncation for Null targets, and updates tests/templates.
NulltoClickhouseEnginewith string round‑trip, proto serialization, hashing, and DDL emission increate_table_query.TryFrom<&str>, regular engine parser) and SQL parser tests to recognizeENGINE = Null(case-insensitive).engine: "Null"viaEngineConfigand map toClickhouseEngine::Null.Null; add helper to match MV target table.NullEngine(); TypeScript generator: emitengine: ClickHouseEngines.Null.py-moose-lib):ClickHouseEngines.NullandNullEngineconfig class.NullEnginerejectsORDER BY,PARTITION BY, andSAMPLE BYinOlapConfig.Nullin discriminated union and conversions.ts-moose-lib):ClickHouseEngines.Null,NullEngineConfig, andNullConfig<T>; validate thatNullforbidsORDER BY/PARTITION BY/SAMPLE BY.engine: "Null"for tables using Null.NullEngineTesttables to TS/Python templates and schema expectations.Written by Cursor Bugbot for commit 88ef238. This will update automatically on new commits. Configure here.