-
Notifications
You must be signed in to change notification settings - Fork 5.5k
test: Add unit tests for expression #26414
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 extends RowExpressionTest.cpp by adding new unit tests for boolean constants (false and null) and logical operators (OR and NOT), and refines formatting in existing tests for improved readability. 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 - here's some feedback:
- A lot of these JSON-based test cases follow the same structure—consider parameterizing the boolean constants (true/false/null) and the special call forms to reduce duplication and improve maintainability.
- The large inline JSON literals make tests harder to read and evolve; think about introducing helper functions or builders for constructing expressions to keep tests concise.
- Some test names (e.g. specialOr, specialNot) could be more descriptive or unified—consider aligning naming conventions so it’s clear these cover logical operators as opposed to generic “special” cases.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- A lot of these JSON-based test cases follow the same structure—consider parameterizing the boolean constants (true/false/null) and the special call forms to reduce duplication and improve maintainability.
- The large inline JSON literals make tests harder to read and evolve; think about introducing helper functions or builders for constructing expressions to keep tests concise.
- Some test names (e.g. specialOr, specialNot) could be more descriptive or unified—consider aligning naming conventions so it’s clear these cover logical operators as opposed to generic “special” cases.
## Individual Comments
### Comment 1
<location> `presto-native-execution/presto_cpp/main/types/tests/RowExpressionTest.cpp:444-445` </location>
<code_context>
TEST_F(RowExpressionTest, char) {
- SystemConfig::instance()->setValue(std::string(SystemConfig::kCharNToVarcharImplicitCast), "true");
+ SystemConfig::instance()->setValue(
+ std::string(SystemConfig::kCharNToVarcharImplicitCast), "true");
std::string str = R"##(
{
</code_context>
<issue_to_address>
**nitpick (testing):** Nitpick: Consider resetting SystemConfig after the test to avoid side effects.
Resetting SystemConfig after the test will prevent unintended interactions with other tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Review automatically exported from Phabricator review in Meta.
7b1c4e5 to
5f413fc
Compare
…o clean up of mapTypeKindToName (prestodb#26414) Summary: X-link: facebookincubator/velox#15264 add more unit tests for expression Reviewed By: bigfootjon Differential Revision: D85356832
Summary: add more unit tests for expression X-link: facebookincubator/velox#15264 Reviewed By: bigfootjon Differential Revision: D85356832 Pulled By: ericyuliu
5f413fc to
cca2c00
Compare
Summary: X-link: prestodb/presto#26414 add more unit tests for expression Pull Request resolved: #15264 Test Plan: unit tests passed Reviewed By: bigfootjon, xiaoxmeng Differential Revision: D85356832 Pulled By: ericyuliu fbshipit-source-id: ed571b79c02ac3093f621903e91490530365ef8f
Summary:
X-link: facebookincubator/velox#15264
add more unit tests for expression
Differential Revision: D85356832