chore(ci): Advance velox and fix Arrow Flight 18#27163
chore(ci): Advance velox and fix Arrow Flight 18#27163czentgr merged 1 commit intoprestodb:masterfrom
Conversation
Reviewer's GuideUpdates native execution build and CI configuration to use a patched Arrow Flight 18-compatible dependency image, adjusts Arrow/Flight build options to apply a custom patch and bundle absl, aligns Velox test utilities with upstream refactors, and removes the NativeSimpleSamplingPercent SQL-invoked function and its associated tests/registration. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
966ed5d to
a7f8a10
Compare
b8658dd to
906a796
Compare
612461e to
e181b11
Compare
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
setup-adapters.sh, consider distinguishing betweenEXTRA_ARROW_PATCHbeing unset vs intentionally set to an empty string so that callers can explicitly disable applying any Arrow patch instead of always falling back toarrow-flight.patchwhen the variable is empty.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `setup-adapters.sh`, consider distinguishing between `EXTRA_ARROW_PATCH` being unset vs intentionally set to an empty string so that callers can explicitly disable applying any Arrow patch instead of always falling back to `arrow-flight.patch` when the variable is empty.
## Individual Comments
### Comment 1
<location path="presto-native-sidecar-plugin/src/test/java/com/facebook/presto/sidecar/TestNativeSidecarPlugin.java" line_range="569" />
<code_context>
assertQuery("SELECT any_values_match(MAP(ARRAY[orderkey], ARRAY[totalprice]), k -> abs(k) > 20) from orders");
assertQuery("SELECT no_values_match(MAP(ARRAY[orderkey], ARRAY[comment]), k -> length(k) > 2) from orders");
assertQuery("SELECT no_keys_match(MAP(ARRAY[comment], ARRAY[custkey]), k -> ends_with(k, 'a')) from orders");
+ assertQuery("select count(1) FROM lineitem l left JOIN orders o ON l.orderkey = o.orderkey JOIN customer c ON o.custkey = c.custkey");
}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider also asserting the same query succeeds when the native inlined SQL feature is disabled
Since the failure assertion in `testNonOverriddenInlinedSqlInvokedFunctionsWhenConfigDisabled` was removed, please also add a positive `assertQuery` for this same SQL when the native inlined SQL feature is disabled (either in that test or a new one). This will verify the query now succeeds via the non-native path and ensure coverage for both modes.
Suggested implementation:
```java
assertQuery("SELECT no_keys_match(MAP(ARRAY[comment], ARRAY[custkey]), k -> ends_with(k, 'a')) from orders");
assertQuery("select count(1) FROM lineitem l left JOIN orders o ON l.orderkey = o.orderkey JOIN customer c ON o.custkey = c.custkey");
}
@Test
public void testLineitemOrdersCustomerJoinWithInlinedSqlDisabled()
{
Session session = createNativeInlinedSqlDisabledSession();
assertQuery(
session,
"select count(1) FROM lineitem l left JOIN orders o ON l.orderkey = o.orderkey JOIN customer c ON o.custkey = c.custkey");
}
```
To fully wire this up, align the `Session` creation with existing conventions in `TestNativeSidecarPlugin`:
1. If there is already a method like `testNonOverriddenInlinedSqlInvokedFunctionsWhenConfigDisabled`, copy the way it creates a session with native inlined SQL disabled (e.g., a helper method or explicit `Session.builder(getSession())...setSystemProperty(..., "false")...build()`), and replace `createNativeInlinedSqlDisabledSession()` with that pattern.
2. If no such helper exists, implement `createNativeInlinedSqlDisabledSession()` in this class, ensuring it:
- Starts from the base session used for native execution tests.
- Sets the system/session property that disables the native inlined SQL feature (use the same property name used elsewhere in this test class for disabling it, or the config property for “inlined sql” in your codebase).
- Returns the configured `Session`.
3. Ensure the class imports `io.prestosql.Session` (or the correct `Session` class for your Presto fork) if it is not already imported.
4. Optionally, if you prefer to keep all “disabled” assertions in `testNonOverriddenInlinedSqlInvokedFunctionsWhenConfigDisabled`, you can instead:
- Move the body of `testLineitemOrdersCustomerJoinWithInlinedSqlDisabled` into that existing test method, using its existing `Session` variable, and
- Remove the new test method introduced above.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...native-sidecar-plugin/src/test/java/com/facebook/presto/sidecar/TestNativeSidecarPlugin.java
Show resolved
Hide resolved
|
For the Iceberg test failures there is a Velox fix available (facebookincubator/velox#16592) that needs to be merged in. Waiting on the update. |
e181b11 to
0f443b1
Compare
|
@amitkdutta @majetideepak @pdabre12 @tdcmeehan Please review. |
|
@czentgr I think we run the @sourcery-ai suggests to run it with both |
|
@unidevel, @tdcmeehan can one of you please approve this PR? Thanks. |
Description
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Summary by Sourcery
Update native execution build scripts to apply a custom Arrow Flight patch and adjust Arrow/Flight dependency configuration.
Build:
Summary by Sourcery
Update native execution to use a patched Arrow Flight build and align with the latest Velox and SQL-invoked function changes.
Enhancements:
Build:
CI:
Tests: