feat(core): apply default nulls last policy for ordering #1262
Conversation
WalkthroughThis change set standardizes the default SQL null ordering to "NULLS LAST" across multiple connectors and engines. It introduces new or updated tests for BigQuery, MySQL, Postgres, Oracle, and MSSQL connectors to verify this behavior, updates Java and Rust core logic to enforce and test the default, and adjusts dependency management and tracing modules as part of a broader upgrade. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant API as Query Endpoint
participant DB as Database Connector
Test->>API: POST /query (ORDER BY column with NULLs)
API->>DB: Execute SQL with ORDER BY (no explicit NULLS)
DB-->>API: Results (NULLs ordered last)
API-->>Test: Response (rows with NULLs last)
sequenceDiagram
participant Core as Core Engine
participant Config as Config Builder
Core->>Config: Build session context
Config->>Config: Set "datafusion.sql_parser.default_null_ordering" = "nulls_last"
Config-->>Core: Session context with default null ordering
Estimated code review effort3 (120 minutes) Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.6)wren-core-legacy/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (24)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
core
bigquery
ibis
rust
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
wren-core-legacy/wren-server/src/main/java/io/wren/server/module/OpenTelemetryModule.java (1)
22-34: LGTM: OpenTelemetry module implementation is correct.The implementation properly creates a no-op OpenTelemetry instance and binds it to the Guice injector. This approach correctly addresses the Airlift 305 tracing requirements while maintaining minimal overhead.
The comment on line 22 clearly explains the rationale for this module.
Consider for future enhancement: Making this configurable so users can optionally enable real tracing when needed, rather than always using no-op.
wren-core-legacy/trino-parser/src/main/java/io/trino/sql/ExpressionFormatter.java (1)
1195-1206: Consider improving code formatting and comment placement.The logic consolidation is correct and aligns with the PR objective. However, the comment placement between case labels could be improved for better readability.
Consider this formatting improvement:
switch (input.getNullOrdering()) { case FIRST: builder.append(" NULLS FIRST"); break; + // Wren engine prefers to use "NULLS LAST" by default null ordering case LAST: - // wren engine prefer to use "NULLS LAST" by default null ordering case UNDEFINED: builder.append(" NULLS LAST"); break; default: throw new UnsupportedOperationException("unknown null ordering: " + input.getNullOrdering()); }This places the comment before the relevant cases and improves readability while maintaining the same functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wren-core-py/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
ibis-server/tests/routers/v2/connector/test_bigquery.py(1 hunks)ibis-server/tests/routers/v2/connector/test_mysql.py(1 hunks)ibis-server/tests/routers/v2/connector/test_postgres.py(1 hunks)ibis-server/tests/routers/v3/connector/bigquery/test_query.py(3 hunks)ibis-server/tests/routers/v3/connector/oracle/conftest.py(1 hunks)ibis-server/tests/routers/v3/connector/oracle/test_query.py(2 hunks)ibis-server/tests/routers/v3/connector/postgres/conftest.py(1 hunks)ibis-server/tests/routers/v3/connector/postgres/test_query.py(2 hunks)wren-core-legacy/pom.xml(2 hunks)wren-core-legacy/trino-parser/src/main/java/io/trino/sql/ExpressionFormatter.java(1 hunks)wren-core-legacy/trino-parser/src/main/java/io/trino/sql/QueryUtil.java(1 hunks)wren-core-legacy/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java(10 hunks)wren-core-legacy/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java(3 hunks)wren-core-legacy/wren-server/pom.xml(1 hunks)wren-core-legacy/wren-server/src/main/java/io/wren/server/WrenServer.java(2 hunks)wren-core-legacy/wren-server/src/main/java/io/wren/server/module/OpenTelemetryModule.java(1 hunks)wren-core-legacy/wren-tests/pom.xml(0 hunks)wren-core-legacy/wren-tests/src/main/java/io/wren/testing/TestingWrenServer.java(0 hunks)wren-core-legacy/wren-tests/src/test/java/io/wren/testing/TestMDLResource.java(1 hunks)wren-core-legacy/wren-tests/src/test/java/io/wren/testing/TestMDLResourceV2.java(3 hunks)wren-core/core/src/mdl/context.rs(1 hunks)wren-core/core/src/mdl/mod.rs(1 hunks)
💤 Files with no reviewable changes (2)
- wren-core-legacy/wren-tests/pom.xml
- wren-core-legacy/wren-tests/src/main/java/io/wren/testing/TestingWrenServer.java
🧰 Additional context used
🧠 Learnings (3)
ibis-server/tests/routers/v3/connector/postgres/test_query.py (1)
Learnt from: goldmedal
PR: Canner/wren-engine#1014
File: wren-core-base/tests/data/mdl.json:8-9
Timestamp: 2024-12-26T04:12:10.888Z
Learning: In test JSON files used for serialization/deserialization, it is acceptable to have empty table references, per the user’s clarification.
ibis-server/tests/routers/v3/connector/oracle/test_query.py (1)
Learnt from: goldmedal
PR: Canner/wren-engine#1014
File: wren-core-base/tests/data/mdl.json:8-9
Timestamp: 2024-12-26T04:12:10.888Z
Learning: In test JSON files used for serialization/deserialization, it is acceptable to have empty table references, per the user’s clarification.
ibis-server/tests/routers/v3/connector/bigquery/test_query.py (1)
Learnt from: goldmedal
PR: Canner/wren-engine#1014
File: wren-core-base/tests/data/mdl.json:8-9
Timestamp: 2024-12-26T04:12:10.888Z
Learning: In test JSON files used for serialization/deserialization, it is acceptable to have empty table references, per the user’s clarification.
🧬 Code Graph Analysis (6)
wren-core-legacy/wren-server/src/main/java/io/wren/server/WrenServer.java (1)
wren-core-legacy/wren-server/src/main/java/io/wren/server/module/OpenTelemetryModule.java (1)
OpenTelemetryModule(23-34)
ibis-server/tests/routers/v3/connector/postgres/test_query.py (8)
ibis-server/tests/routers/v3/connector/bigquery/test_query.py (2)
test_order_by_nulls_last(353-388)manifest_str(76-77)ibis-server/tests/routers/v2/connector/test_bigquery.py (2)
test_order_by_nulls_last(497-511)manifest_str(73-74)ibis-server/tests/routers/v2/connector/test_mysql.py (2)
test_order_by_nulls_last(478-493)manifest_str(83-84)ibis-server/tests/routers/v3/connector/oracle/test_query.py (2)
test_order_by_nulls_last(225-260)manifest_str(82-83)ibis-server/tests/routers/v2/connector/test_postgres.py (2)
test_order_by_nulls_last(1029-1044)manifest_str(125-126)ibis-server/tests/conftest.py (1)
client(18-23)ibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.py (1)
manifest_str(30-31)ibis-server/tests/routers/v3/connector/postgres/conftest.py (1)
connection_info(45-52)
wren-core-legacy/wren-tests/src/test/java/io/wren/testing/TestMDLResourceV2.java (2)
wren-core-base/manifest-macro/src/lib.rs (1)
manifest(26-56)mcp-server/app/dto.py (1)
Manifest(41-47)
ibis-server/tests/routers/v2/connector/test_mysql.py (4)
ibis-server/tests/routers/v2/connector/test_bigquery.py (2)
test_order_by_nulls_last(497-511)manifest_str(73-74)ibis-server/tests/routers/v2/connector/test_postgres.py (2)
test_order_by_nulls_last(1029-1044)manifest_str(125-126)ibis-server/tests/conftest.py (1)
client(18-23)ibis-server/tests/routers/v2/connector/test_clickhouse.py (1)
manifest_str(109-110)
ibis-server/tests/routers/v3/connector/oracle/test_query.py (7)
ibis-server/tests/routers/v3/connector/bigquery/test_query.py (2)
test_order_by_nulls_last(353-388)manifest_str(76-77)ibis-server/tests/routers/v2/connector/test_bigquery.py (2)
test_order_by_nulls_last(497-511)manifest_str(73-74)ibis-server/tests/routers/v2/connector/test_mysql.py (2)
test_order_by_nulls_last(478-493)manifest_str(83-84)ibis-server/tests/routers/v2/connector/test_postgres.py (2)
test_order_by_nulls_last(1029-1044)manifest_str(125-126)ibis-server/tests/conftest.py (1)
client(18-23)ibis-server/tests/routers/v3/connector/oracle/test_function.py (1)
manifest_str(31-32)ibis-server/tests/routers/v3/connector/oracle/conftest.py (1)
connection_info(102-111)
ibis-server/tests/routers/v3/connector/bigquery/test_query.py (5)
ibis-server/tests/routers/v3/connector/oracle/test_query.py (2)
test_order_by_nulls_last(225-260)manifest_str(82-83)ibis-server/tests/routers/v3/connector/postgres/test_query.py (2)
test_order_by_nulls_last(726-761)manifest_str(134-135)ibis-server/tests/conftest.py (1)
client(18-23)ibis-server/tests/routers/v3/connector/bigquery/test_functions.py (1)
manifest_str(31-32)ibis-server/tests/routers/v3/connector/bigquery/conftest.py (1)
connection_info(24-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: ci
- GitHub Check: cargo test (macos)
- GitHub Check: cargo test (macos-aarch64)
- GitHub Check: cargo check
- GitHub Check: cargo test (win64)
- GitHub Check: test
🔇 Additional comments (40)
wren-core-legacy/wren-server/pom.xml (1)
57-60: LGTM: OpenTelemetry dependency addition is correct.The addition of the
io.opentelemetry:opentelemetry-apidependency properly supports the newOpenTelemetryModuleintroduced in this change set.wren-core-legacy/wren-server/src/main/java/io/wren/server/WrenServer.java (2)
29-29: LGTM: Import replacement is correct.The import change from
EventModuletoOpenTelemetryModuleproperly aligns with the Airlift 305 upgrade and tracing infrastructure migration.
48-48: LGTM: Module instantiation correctly updated.The replacement of
EventModulewithOpenTelemetryModulein the module list is consistent with the import change and properly supports the new tracing infrastructure.wren-core-legacy/pom.xml (1)
67-67: Manual Compatibility Verification: Airlift 305We’ve confirmed extensive use of Airlift APIs throughout the codebase—upgrading from 269 to 305 may introduce breaking changes or renamed/removed modules. Please cross-check Airlift 305 release notes for any API or module changes (e.g., HttpServerModule, JaxrsModule, JsonModule, NodeModule, AbstractConfigurationAwareModule, ConfigBinder).
Key areas to review:
- wren-core-legacy/wren-server/src/main/java/io/wren/server/WrenServer.java
- wren-core-legacy/wren-server/src/main/java/io/wren/server/module/*Module.java
- wren-core-legacy/wren-tests/** (all classes importing io.airlift.*)
- wren-core-legacy/wren-base/** and wren-main/** (configuration & logging imports)
No EventModule references remain.
wren-core/core/src/mdl/context.rs (1)
65-68: LGTM! Correctly implements default NULLS LAST policy.The configuration setting properly establishes the default null ordering behavior in DataFusion's SQL parser. This change aligns with the PR objective and ensures consistent null ordering across the query engine.
ibis-server/tests/routers/v3/connector/oracle/conftest.py (1)
91-96: LGTM! Well-structured test fixture for null ordering validation.The
null_testtable setup correctly provides test data with NULL values to support null ordering behavior tests. The use of quoted identifiers and proper Oracle SQL syntax is appropriate.wren-core-legacy/wren-tests/src/test/java/io/wren/testing/TestMDLResource.java (1)
100-100: LGTM! Improved error message matching robustness.The simplified regex pattern with the DOTALL flag (
(?s)) makes the test more resilient to formatting changes in error messages while maintaining the essential validation.wren-core-legacy/trino-parser/src/main/java/io/trino/sql/QueryUtil.java (1)
174-174: LGTM! Correctly implements default NULLS LAST for ascending sorts.The change from
NullOrdering.UNDEFINEDtoNullOrdering.LASTproperly establishes the default null ordering behavior for ascending sorts, directly supporting the PR objective.ibis-server/tests/routers/v3/connector/postgres/conftest.py (1)
32-38: LGTM! Consistent test fixture for null ordering validation.The
null_testtable setup provides appropriate test data with NULL values for validating null ordering behavior. The implementation is clean and consistent with similar fixtures across other connectors.ibis-server/tests/routers/v2/connector/test_bigquery.py (1)
497-511: LGTM! Test correctly validates default NULLS LAST behavior.The test implementation properly verifies that when no explicit null ordering is specified, null values are ordered last by default. The test data and assertions are well-structured to validate this behavior across the BigQuery connector.
ibis-server/tests/routers/v2/connector/test_postgres.py (1)
1029-1044: Confirm PostgreSQL default null ordering isn’t masking connector-specific behavior.We ran repository-wide searches for any explicit
NULLS FIRST/LASTclauses and found no matches. As PostgreSQL by default sortsNULLvalues last in ascending order, this test may simply be exercising native behavior rather than a connector override.
Please verify that:
- The goal is to enforce or customize null ordering in the PostgreSQL connector (not just rely on the database default).
- If so, ensure the connector layer explicitly applies
NULLS LAST(or document why the default is sufficient).- Otherwise, consider whether this test is redundant.
ibis-server/tests/routers/v2/connector/test_mysql.py (1)
478-494: LGTM! Test correctly validates default NULLS LAST ordering behavior.The test implementation is consistent with similar tests across other connectors (BigQuery, Postgres, Oracle) and correctly validates that NULL values are ordered last by default when no explicit NULLS FIRST/LAST is specified. The inline VALUES clause approach provides a clean way to test this behavior without requiring additional test data setup.
wren-core-legacy/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java (3)
20-20: Appropriate import addition for JUnit 5 @disabled annotation.
46-49: Good practice: Clear explanation for test disabling.The comment clearly explains why the test is disabled - Wren engine doesn't support SQL-to-AST roundtrip conversion, which is a deliberate architectural divergence from Trino. This helps future maintainers understand the reasoning.
348-351: Consistent test disabling with clear rationale.The same rationale and approach as the previous disabled test, maintaining consistency in the codebase.
ibis-server/tests/routers/v3/connector/oracle/test_query.py (2)
66-76: Well-structured test model for null ordering validation.The "null_test" model is appropriately defined with simple integer and varchar columns to support null ordering tests. The table reference follows Oracle naming conventions.
225-260: Comprehensive null ordering test with both ASC and DESC scenarios.The test implementation is thorough and well-structured:
- Tests both ascending and descending order scenarios
- Uses quoted identifiers in the second query, which is appropriate for Oracle
- Properly disables DuckDB fallback to ensure Oracle connector behavior is tested
- Expected results correctly validate that NULL values are ordered last in both cases
- Consistent with similar tests in other v3 connectors (BigQuery)
This provides robust validation of the default NULLS LAST policy for Oracle.
wren-core-legacy/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java (16)
200-200: LGTM! Import addition is correct.The import of
@Disabledannotation is properly added to support disabling specific test methods throughout the file.
251-251: LGTM! Static import addition supports null ordering changes.The static import of
LASTfromSortItem.NullOrderingis correctly added to support the updated test expectations for default null ordering behavior.
642-643: LGTM! Test properly disabled for Wren engine compatibility.The
@Disabledannotation is correctly applied to exclude this test that doesn't work with Wren engine's SQL-to-AST roundtrip behavior.
1147-1148: LGTM! Clear explanatory comments added.The comments provide valuable context explaining why tests are disabled due to Wren engine's different SQL-to-AST roundtrip expectations.
1149-1150: LGTM! Test properly disabled for consistency.The
@Disabledannotation is consistently applied to thetestSelectWithOrderBymethod following the same pattern as other disabled tests.
2769-2770: LGTM! Consistent explanatory comments.The comments maintain consistency with other disabled test explanations throughout the file.
2771-2772: LGTM! Test disabling follows established pattern.The
@Disabledannotation is consistently applied to thetestShowStatsForQuerymethod.
2994-2995: LGTM! Documentation consistency maintained.The explanatory comments are consistently applied across all disabled tests.
2996-2997: LGTM! Consistent test disabling pattern.The
@Disabledannotation is properly applied to thetestAggregationWithOrderBymethod.
3006-3006: LGTM! Null ordering standardized to LAST.The change from
UNDEFINEDtoLASTforSortItem.NullOrderingaligns with the PR objective to standardize default null ordering to "NULLS LAST" behavior.
3018-3018: LGTM! Consistent null ordering standardization.The change from
UNDEFINEDtoLASTmaintains consistency with the null ordering standardization across test expectations.
3480-3481: LGTM! Consistent documentation pattern.The explanatory comments follow the same pattern as other disabled tests in the file.
3482-3483: LGTM! Test disabling pattern maintained.The
@Disabledannotation is consistently applied to thetestWindowClausemethod.
3506-3506: LGTM! Null ordering change maintains consistency.The change from
UNDEFINEDtoLASTin theOrderByconstruction is consistent with the null ordering standardization throughout the file.
3513-3514: LGTM! Documentation consistency maintained.The explanatory comments are consistently applied across all disabled tests.
3515-3516: LGTM! Final test disabling completes the pattern.The
@Disabledannotation is consistently applied to thetestWindowFrameWithPatternRecognitionmethod, completing the pattern of disabled tests.wren-core-legacy/wren-tests/src/test/java/io/wren/testing/TestMDLResourceV2.java (2)
309-309: LGTM: Clean formatting improvementsThese formatting fixes improve code consistency by removing trailing spaces and cleaning up string literals.
Also applies to: 329-329, 361-362
364-473: Excellent test coverage for default null ordering behaviorThis test comprehensively validates the new default "NULLS LAST" policy across three key scenarios:
- Default ascending order →
ASC NULLS LAST- Descending order →
DESC NULLS LAST- Explicit nulls first →
ASC NULLS FIRSTThe test structure follows established patterns and provides thorough verification of the SQL generation logic. The assertions correctly validate that the default behavior applies "NULLS LAST" when no explicit null ordering is specified, while respecting explicit "NULLS FIRST" directives.
ibis-server/tests/routers/v3/connector/postgres/test_query.py (2)
110-120: Well-structured model definition for null ordering testsThe
null_testmodel follows the established manifest pattern and provides the necessary table structure for testing null ordering behavior. The simple schema withidandlettercolumns is appropriate for the test scenarios.
726-761: Integration test for NULL ordering is correctly set upThe
null_testfixture inibis-server/tests/routers/v3/connector/postgres/conftest.pyconfirms:
- Table creation:
CREATE TABLE null_test (id INT, letter TEXT)- Data insertion:
(1, 'one'), (2, 'two'), (NULL, 'three')With the setup verified, the
test_order_by_nulls_lastintegration test is valid and can be approved.wren-core/core/src/mdl/mod.rs (1)
2872-2931: Excellent test coverage for default NULLS LAST behavior!This test function comprehensively validates the default null ordering functionality across multiple scenarios:
- Basic ORDER BY (defaulting to ASC NULLS LAST)
- Explicit ASC/DESC directions (both get NULLS LAST by default)
- Explicit NULLS FIRST/LAST preservation
- Multiple column ordering with mixed directions
- Complex scenarios with both default and explicit null ordering
The use of snapshot testing ensures exact SQL transformation verification, which is critical for this feature. The test implementation aligns perfectly with the PR objective of standardizing default null ordering to "NULLS LAST" across connectors.
ibis-server/tests/routers/v3/connector/bigquery/test_query.py (2)
7-7: LGTM: Import addition is correct.The import of
X_WREN_FALLBACK_DISABLEis properly placed and necessary for the new null ordering test to ensure BigQuery native behavior is tested.
60-70: LGTM: Model definition is properly structured.The "null_test" model definition correctly follows the established manifest pattern with appropriate column types for testing null ordering behavior.
|
BigQuery has been tested locally |
goldmedal
force-pushed
the
feat/default-null-last
branch
from
16c9c3c to
0b14820
Compare
|
BigQuery has been tested locally. Let's ignore the CI bigquery failed. |
|
Thanks @goldmedal |
2 participants
Description
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation