-
Notifications
You must be signed in to change notification settings - Fork 5.5k
[native] Add support for setting sessionStartTime in VeloxQueryConfig. #26102
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
Conversation
Reviewer's GuideThis PR enables propagation of session start time into VeloxQueryConfig by extending the configuration update logic and adds comprehensive unit tests to verify correct handling of various start time values. Sequence diagram for setting sessionStartTime in VeloxQueryConfigsequenceDiagram
participant S as Session
participant P as PrestoToVeloxQueryConfig
participant Q as QueryConfig
S->>P: Provide startTime
P->>Q: Set queryConfigs[kSessionStartTime] = to_string(startTime)
P->>Q: Set other configs (e.g., source)
Class diagram for updated VeloxQueryConfig session start time supportclassDiagram
class Session {
+startTime: optional<int64_t>
+source: optional<string>
}
class PrestoToVeloxQueryConfig {
+updateFromSessionConfigs(session: Session, queryConfigs: map<string, string>)
}
class QueryConfig {
<<static>>
+kSessionStartTime: string
+kSource: string
}
Session --> PrestoToVeloxQueryConfig : used by
PrestoToVeloxQueryConfig --> QueryConfig : uses constants
PrestoToVeloxQueryConfig : +updateFromSessionConfigs()
PrestoToVeloxQueryConfig : +queryConfigs[kSessionStartTime]
PrestoToVeloxQueryConfig : +queryConfigs[kSource]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-native-execution/presto_cpp/main/tests/PrestoToVeloxQueryConfigTest.cpp:487-499` </location>
<code_context>
+
+ EXPECT_EQ(0, veloxConfig3.sessionStartTimeMs());
+
+ // Test with negative start time (valid edge case)
+ session.startTime = -1000;
+ auto veloxConfig4 = toVeloxConfigs(session);
+
+ EXPECT_EQ(-1000, veloxConfig4.sessionStartTimeMs());
+
+ // Test with maximum value
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for unset/absent sessionStartTime.
Testing the case where session.startTime is unset will confirm correct handling of missing values.
```suggestion
// Test with zero start time (valid edge case)
session.startTime = 0;
auto veloxConfig3 = toVeloxConfigs(session);
EXPECT_EQ(0, veloxConfig3.sessionStartTimeMs());
// Test with unset/absent start time
session.startTime = {};
auto veloxConfigUnset = toVeloxConfigs(session);
EXPECT_EQ(0, veloxConfigUnset.sessionStartTimeMs());
// Test with negative start time (valid edge case)
session.startTime = -1000;
auto veloxConfig4 = toVeloxConfigs(session);
EXPECT_EQ(-1000, veloxConfig4.sessionStartTimeMs());
// Test with maximum value
```
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
// Test with zero start time (valid edge case) | ||
session.startTime = 0; | ||
auto veloxConfig3 = toVeloxConfigs(session); | ||
|
||
EXPECT_EQ(0, veloxConfig3.sessionStartTimeMs()); | ||
|
||
// Test with negative start time (valid edge case) | ||
session.startTime = -1000; | ||
auto veloxConfig4 = toVeloxConfigs(session); | ||
|
||
EXPECT_EQ(-1000, veloxConfig4.sessionStartTimeMs()); | ||
|
||
// Test with maximum value |
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.
suggestion (testing): Consider adding a test for unset/absent sessionStartTime.
Testing the case where session.startTime is unset will confirm correct handling of missing values.
// Test with zero start time (valid edge case) | |
session.startTime = 0; | |
auto veloxConfig3 = toVeloxConfigs(session); | |
EXPECT_EQ(0, veloxConfig3.sessionStartTimeMs()); | |
// Test with negative start time (valid edge case) | |
session.startTime = -1000; | |
auto veloxConfig4 = toVeloxConfigs(session); | |
EXPECT_EQ(-1000, veloxConfig4.sessionStartTimeMs()); | |
// Test with maximum value | |
// Test with zero start time (valid edge case) | |
session.startTime = 0; | |
auto veloxConfig3 = toVeloxConfigs(session); | |
EXPECT_EQ(0, veloxConfig3.sessionStartTimeMs()); | |
// Test with unset/absent start time | |
session.startTime = {}; | |
auto veloxConfigUnset = toVeloxConfigs(session); | |
EXPECT_EQ(0, veloxConfigUnset.sessionStartTimeMs()); | |
// Test with negative start time (valid edge case) | |
session.startTime = -1000; | |
auto veloxConfig4 = toVeloxConfigs(session); | |
EXPECT_EQ(-1000, veloxConfig4.sessionStartTimeMs()); | |
// Test with maximum value |
66748f3
to
c39635d
Compare
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 @kgpai
@amitkdutta Time tests are failing :) |
# Conflicts: # presto-native-execution/presto_cpp/main/tests/PrestoToVeloxQueryConfigTest.cpp
c39635d
to
8df23d6
Compare
@amitkdutta I dont believe these failures are https://github.com/prestodb/presto/actions/runs/17923502131/job/50963889609?pr=26102 are related to this change ? |
Description
This diff pushes session start time to VeloxQueryConfig.
Motivation and Context
This is required for queries like current_date, localtime, current_time etc which use the session start time instead of wall clock time.
Impact
N/A
Test Plan
Unite test.
If release note is NOT required, use: