Add session properties for scaled table scan configs #24284
+68
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
natashasehgal
changed the title
|
The change to the Java documentation should go together with this change to the coordinator. You can include your original patch to presto-docs/src/main/sphinx/presto_cpp/properties-session.rst in this PR. Logically, supposed the OSS community see this change and would like to look for more info, they could find the Java documentation in the codebase. Usually the files under presto-native-execution belong to the native C++ worker and are packaged and released together. Everything else belongs to the Java coordinator. |
|
We always have the Description section filled out. Other sections could be possibly skipped if not applicable. You can move your content under Motivation to Description. |
8 tasks
natashasehgal
changed the title
gggrace14
previously approved these changes
Dec 20, 2024
|
Thank you @natashasehgal for making the change! |
natashasehgal
force-pushed
the
T210844289
branch
from
5340b2f to
2daa462
Compare
zation99
reviewed
Dec 31, 2024
| When enabled, Presto will attempt to scale up table scans to improve performance. | ||
|
|
||
| ``native_table_scan_scale_up_memory_usage_ratio`` | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
need to make it the same length as line 432
|
Pls add to release notes. |
|
where's the implementation for this feature? How will it get used? |
@rschlussel I have a follow up PR for the Native worker implementation to avoid issues, as Java and C++ packages are released separately |
rschlussel
reviewed
Jan 2, 2025
There was a problem hiding this comment.
@rschlussel I have a follow up PR for the Native worker implementation to avoid issues, as Java and C++ packages are released separately
Got it, thanks! What problem does splitting this into 2 prs avoid? Without context about this, it seems fine that they are released separately even if 1 pr contains changes for both. Either way, we'd need to make sure the coordinator has the necessary changes once we take in the worker changes.
...n/java/com/facebook/presto/sessionpropertyproviders/NativeWorkerSessionPropertyProvider.java
Outdated
Show resolved
Hide resolved
steveburnett
previously approved these changes
Jan 3, 2025
@rschlussel for this particular change separate releases are ok. There's no dependency. Any preference between
|
natashasehgal
changed the title
rschlussel
reviewed
Jan 6, 2025
There was a problem hiding this comment.
Don't know if the failures are related, but they are three null pointer exceptions, so certainly should be looked at by someone. I'll restart the test, and we'll see if it's flaky or related.
2025-01-03T20:58:50.1224008Z [ERROR] Tests run: 8, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 59.579 s <<< FAILURE! - in com.facebook.presto.spark.TestPrestoSparkNativeArrayFunctionQueries
2025-01-03T20:58:50.1225948Z [ERROR] com.facebook.presto.spark.TestPrestoSparkNativeArrayFunctionQueries.testArrayMatch Time elapsed: 4.407 s <<< FAILURE!
2025-01-03T20:58:50.1227479Z java.lang.AssertionError: Execution of 'actual' query failed: SELECT all_match(shuffle(quantities), x -> (x > 500.0)) FROM orders_ex
2025-01-03T20:58:50.1228417Z at org.testng.Assert.fail(Assert.java:98)
2025-01-03T20:58:50.1229314Z at com.facebook.presto.tests.QueryAssertions.assertQuery(QueryAssertions.java:178)
2025-01-03T20:58:50.1230666Z at com.facebook.presto.tests.QueryAssertions.assertQuery(QueryAssertions.java:106)
2025-01-03T20:58:50.1231830Z at com.facebook.presto.tests.AbstractTestQueryFramework.assertQuery(AbstractTestQueryFramework.java:169)
2025-01-03T20:58:50.1232913Z at com.facebook.presto.tests.AbstractTestQueryFramework.assertQuery(AbstractTestQueryFramework.java:164)
2025-01-03T20:58:50.1234282Z at com.facebook.presto.nativeworker.AbstractTestNativeArrayFunctionQueries.testArrayMatch(AbstractTestNativeArrayFunctionQueries.java:63)
2025-01-03T20:58:50.1235706Z at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2025-01-03T20:58:50.1236746Z at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
2025-01-03T20:58:50.1238055Z at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2025-01-03T20:58:50.1239005Z at java.base/java.lang.reflect.Method.invoke(Method.java:566)
2025-01-03T20:58:50.1239910Z at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:135)
2025-01-03T20:58:50.1240928Z at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:673)
2025-01-03T20:58:50.1241838Z at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:220)
2025-01-03T20:58:50.1242751Z at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
2025-01-03T20:58:50.1243870Z at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:945)
2025-01-03T20:58:50.1244868Z at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:193)
2025-01-03T20:58:50.1245909Z at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
2025-01-03T20:58:50.1246946Z at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
2025-01-03T20:58:50.1247966Z at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
2025-01-03T20:58:50.1249053Z at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
2025-01-03T20:58:50.1249895Z at java.base/java.lang.Thread.run(Thread.java:829)
2025-01-03T20:58:50.1250405Z Caused by: java.lang.NullPointerException
2025-01-03T20:58:50.1251380Z at com.facebook.presto.spark.util.PrestoSparkFailureUtils.toPrestoSparkFailure(PrestoSparkFailureUtils.java:59)
2025-01-03T20:58:50.1252962Z at com.facebook.presto.spark.execution.AbstractPrestoSparkQueryExecution.execute(AbstractPrestoSparkQueryExecution.java:395)
2025-01-03T20:58:50.1254494Z at com.facebook.presto.spark.PrestoSparkQueryRunner.executeWithStrategies(PrestoSparkQueryRunner.java:548)
2025-01-03T20:58:50.1255759Z at com.facebook.presto.spark.PrestoSparkQueryRunner.execute(PrestoSparkQueryRunner.java:522)
2025-01-03T20:58:50.1256825Z at com.facebook.presto.tests.QueryAssertions.assertQuery(QueryAssertions.java:175)
2025-01-03T20:58:50.1257499Z ... 19 more
2025-01-03T20:46:41.7396293Z [ERROR] com.facebook.presto.spark.TestPrestoSparkNativeJoinQueries.testBucketedInnerJoin[Session{queryId=20250103_204516_00000_xnsj9, transactionId=Optional.empty, user=user, source=test, catalog=hive, schema=tpch, timeZoneKey=Pacific/Apia, locale=en, remoteUserAddress=address, userAgent=agent, clientTags=[], resourceEstimates=ResourceEstimates{executionTime=Optional.empty, cpuTime=Optional.empty, peakMemory=Optional.empty, peakTaskMemory=Optional.empty}, startTime=1735937116603}](1) Time elapsed: 6.216 s <<< FAILURE!
2025-01-03T20:46:41.7400605Z java.lang.AssertionError: Execution of 'actual' query failed: SELECT b.name, c.name FROM customer_bucketed b, customer c WHERE b.name=c.name
2025-01-03T20:46:41.7401857Z at org.testng.Assert.fail(Assert.java:98)
2025-01-03T20:46:41.7402592Z at com.facebook.presto.tests.QueryAssertions.assertQuery(QueryAssertions.java:178)
2025-01-03T20:46:41.7403514Z at com.facebook.presto.tests.QueryAssertions.assertQuery(QueryAssertions.java:106)
2025-01-03T20:46:41.7404582Z at com.facebook.presto.tests.AbstractTestQueryFramework.assertQuery(AbstractTestQueryFramework.java:169)
2025-01-03T20:46:41.7405892Z at com.facebook.presto.nativeworker.AbstractTestNativeJoinQueries.testBucketedInnerJoin(AbstractTestNativeJoinQueries.java:61)
2025-01-03T20:46:41.7407139Z at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2025-01-03T20:46:41.7408189Z at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
2025-01-03T20:46:41.7409824Z at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2025-01-03T20:46:41.7410881Z at java.base/java.lang.reflect.Method.invoke(Method.java:566)
2025-01-03T20:46:41.7411852Z at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:135)
2025-01-03T20:46:41.7412951Z at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:673)
2025-01-03T20:46:41.7413765Z at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:220)
2025-01-03T20:46:41.7414554Z at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
2025-01-03T20:46:41.7415378Z at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:945)
2025-01-03T20:46:41.7416320Z at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:193)
2025-01-03T20:46:41.7417343Z at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
2025-01-03T20:46:41.7418742Z at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
2025-01-03T20:46:41.7419806Z at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
2025-01-03T20:46:41.7420859Z at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
2025-01-03T20:46:41.7421744Z at java.base/java.lang.Thread.run(Thread.java:829)
2025-01-03T20:46:41.7422319Z Caused by: java.lang.NullPointerException
2025-01-03T20:46:41.7422901Z at com.facebook.presto.spark.util.PrestoSparkFailureUtils.toPrestoSparkFailure(PrestoSparkFailureUtils.java:59)
2025-01-03T20:46:41.7423792Z at com.facebook.presto.spark.execution.AbstractPrestoSparkQueryExecution.execute(AbstractPrestoSparkQueryExecution.java:395)
2025-01-03T20:46:41.7425262Z at com.facebook.presto.spark.PrestoSparkQueryRunner.executeWithStrategies(PrestoSparkQueryRunner.java:548)
2025-01-03T20:46:41.7426463Z at com.facebook.presto.spark.PrestoSparkQueryRunner.execute(PrestoSparkQueryRunner.java:522)
2025-01-03T20:46:41.7427480Z at com.facebook.presto.tests.QueryAssertions.assertQuery(QueryAssertions.java:175)
2025-01-03T20:46:41.7428128Z ... 18 more
2025-01-03T20:46:41.7428282Z
2025-01-03T21:14:45.0161596Z [ERROR] Tests run: 23, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 867.866 s <<< FAILURE! - in com.facebook.presto.spark.TestPrestoSparkNativeWindowQueries
2025-01-03T21:14:45.0163231Z [ERROR] com.facebook.presto.spark.TestPrestoSparkNativeWindowQueries.testLagOrderDate Time elapsed: 91.404 s <<< FAILURE!
2025-01-03T21:14:45.0165129Z java.lang.AssertionError: Execution of 'actual' query failed: SELECT lag(orderdate) OVER (PARTITION BY orderkey ORDER BY totalprice RANGE BETWEEN 5 FOLLOWING AND 10 FOLLOWING) FROM orders
2025-01-03T21:14:45.0166359Z at org.testng.Assert.fail(Assert.java:98)
2025-01-03T21:14:45.0167070Z at com.facebook.presto.tests.QueryAssertions.assertQuery(QueryAssertions.java:178)
2025-01-03T21:14:45.0168038Z at com.facebook.presto.tests.QueryAssertions.assertQuery(QueryAssertions.java:106)
2025-01-03T21:14:45.0169213Z at com.facebook.presto.tests.AbstractTestQueryFramework.assertQuery(AbstractTestQueryFramework.java:169)
2025-01-03T21:14:45.0170534Z at com.facebook.presto.tests.AbstractTestQueryFramework.assertQuery(AbstractTestQueryFramework.java:164)
2025-01-03T21:14:45.0172044Z at com.facebook.presto.nativeworker.AbstractTestNativeWindowQueries.testWindowFunction(AbstractTestNativeWindowQueries.java:135)
2025-01-03T21:14:45.0174027Z at com.facebook.presto.nativeworker.AbstractTestNativeWindowQueries.testLagOrderDate(AbstractTestNativeWindowQueries.java:245)
2025-01-03T21:14:45.0175525Z at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2025-01-03T21:14:45.0176713Z at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
2025-01-03T21:14:45.0177962Z at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2025-01-03T21:14:45.0178956Z at java.base/java.lang.reflect.Method.invoke(Method.java:566)
2025-01-03T21:14:45.0179921Z at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:135)
2025-01-03T21:14:45.0180990Z at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:673)
2025-01-03T21:14:45.0181898Z at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:220)
2025-01-03T21:14:45.0182833Z at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
2025-01-03T21:14:45.0183848Z at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:945)
2025-01-03T21:14:45.0184897Z at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:193)
2025-01-03T21:14:45.0185937Z at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
2025-01-03T21:14:45.0186941Z at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
2025-01-03T21:14:45.0188053Z at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
2025-01-03T21:14:45.0189128Z at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
2025-01-03T21:14:45.0189940Z at java.base/java.lang.Thread.run(Thread.java:829)
2025-01-03T21:14:45.0190730Z Caused by: java.lang.NullPointerException
2025-01-03T21:14:45.0191665Z at com.facebook.presto.spark.util.PrestoSparkFailureUtils.toPrestoSparkFailure(PrestoSparkFailureUtils.java:59)
2025-01-03T21:14:45.0193200Z at com.facebook.presto.spark.execution.AbstractPrestoSparkQueryExecution.execute(AbstractPrestoSparkQueryExecution.java:395)
2025-01-03T21:14:45.0194745Z at com.facebook.presto.spark.PrestoSparkQueryRunner.executeWithStrategies(PrestoSparkQueryRunner.java:548)
2025-01-03T21:14:45.0196148Z at com.facebook.presto.spark.PrestoSparkQueryRunner.execute(PrestoSparkQueryRunner.java:522)
2025-01-03T21:14:45.0197223Z at com.facebook.presto.tests.QueryAssertions.assertQuery(QueryAssertions.java:175)
2025-01-03T21:14:45.0197898Z ... 20 more
2025-01-03T21:14:45.0198058Z
| !nativeExecution), | ||
| doubleProperty( | ||
| NATIVE_TABLE_SCAN_SCALE_UP_MEMORY_USAGE_RATIO, | ||
| "The query memory usage ratio used by scan controller to decide if it can " + |
There was a problem hiding this comment.
would be good to add validation so we fail fast if an invalid property is sent. you can look at https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java#L1026-L1034 for an example of how to do that.
Does cpp have any validation options for session properties?
There was a problem hiding this comment.
I don't see any Validation File specific to cpp. I do see a Test file, where I've added these session properties.
From the file you shared "Native execution related session properties are temporarily put here." Should I add these native configurations there as well? Does it could as execution-related?
There was a problem hiding this comment.
no, where you have them now is good. I think the ones in that file are things that the coordinator needs to know about for planning. The configs you added the coordinator doesn't need to use at all. it just needs to know about them so it can pass them through to the worker.
|
Also, please squash all the commits together into one. |
natashasehgal
force-pushed
the
T210844289
branch
from
f86ba49 to
4cb7450
Compare
@rschlussel how are you restarting tests? After pushing minor range fix in comment, I'm seeing an out of space error in Linux Build test. I would like to restart this failing test. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add session properties to Java coordinator and native works to scale table scan
Motivation and Context
Impact
Low impact
Test Plan
Contributor checklist
Release Notes
Session changes
2428424284