Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

add hint case #22665

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

Open
Ariznawlll wants to merge 12 commits into matrixorigin:main
base: main
Choose a base branch
Loading
from Ariznawlll:add_case_view_defination

Conversation

@Ariznawlll
Copy link
Contributor

@Ariznawlll Ariznawlll commented Oct 23, 2025
edited
Loading

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

add hint case

issue #
#22466

What this PR does / why we need it:

add hint case


PR Type

Tests


Description

  • Add comprehensive test suite for hint rewriting functionality

  • Test basic single and multi-table rewrite rules with JSON hints

  • Test complex queries including aggregations, grouping, window functions

  • Test edge cases: empty rules, special characters, case sensitivity

  • Test error handling: invalid JSON, syntax errors, non-existent tables


Diagram Walkthrough

flowchart LR
 A["Test Suite"] --> B["Basic Rewrites"]
 A --> C["Complex Queries"]
 A --> D["Edge Cases"]
 A --> E["Error Handling"]
 B --> B1["Single Table"]
 B --> B2["Multi Table"]
 C --> C1["Aggregations"]
 C --> C2["Window Functions"]
 C --> C3["Subqueries"]
 D --> D1["Empty Rules"]
 D --> D2["Special Characters"]
 D --> D3["Case Sensitivity"]
 E --> E1["Invalid JSON"]
 E --> E2["SQL Syntax Errors"]
 E --> E3["Non-existent Tables"]
Loading

File Walkthrough

Relevant files
Tests
hint.sql
Comprehensive hint rewriting test suite

test/distributed/cases/hint/hint.sql

  • Create comprehensive test suite with 529 lines covering hint rewriting
    functionality
  • Test basic single-table and multi-table rewrite rules with JSON hint
    syntax
  • Test complex query scenarios: aggregations, GROUP BY, window
    functions, subqueries, UNION
  • Test edge cases: empty rewrite rules, special characters in table
    names, case sensitivity
  • Test error handling: malformed JSON, SQL syntax errors, non-existent
    table references, recursive detection
  • Include performance testing with large table operations
+529/-0
hint.result
Expected results for hint rewriting tests

test/distributed/cases/hint/hint.result

  • Add expected test results for 515 test cases covering hint rewriting
    functionality
  • Include successful query results for basic rewrites with filtered data
  • Document expected results for complex aggregation and window function
    queries
  • Include error messages for invalid JSON formats, SQL syntax errors,
    and non-existent tables
  • Mark results as unknown for tests related to issue [Bug]: hint report ' ambiguouse column reference to column'. #22662 pending
    resolution
+515/-0

Copy link

CLAassistant commented Oct 23, 2025
edited
Loading

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ Ariznawlll
Ariznawl@163.com


Ariznawl@163.com seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@matrix-meow matrix-meow added the size/XL Denotes a PR that changes [1000, 1999] lines label Oct 23, 2025
Copy link

qodo-merge-pro bot commented Oct 23, 2025
edited
Loading

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Malformed input handling

Description: The test suite embeds intentionally malformed JSON and SQL in hints, which is acceptable
for testing but could be risky if such patterns were copied into production code; ensure
the engine robustly validates and rejects these without executing partial inputs.
hint.sql [389-401]

Referred Code
/*+ {
 "rewrites": {
 "hint_test.users": "SELECT * FROM users"
} */
select * from users;
-- missing quotation marks
/*+ {
 rewrites: {
 "hint_test.users": "SELECT * FROM users"
 }
} */
select * from users;
Ticket Compliance
🟡
🎫 #22466
🟢 Allow users to provide query hint comments using /*+ ... */ or /*! ... */ that specify
rewrite rules.
Support mapping a referenced table to a provided SELECT statement via a JSON "rewrites"
map.
Apply rewrite rules during parse/plan so that listed tables are replaced with the
specified SELECTs.
Do not recurse rewrite expansion; within rewrite rules, tables must resolve to real tables
or views only.
Support mapping a table name to a real table with the same name (no-op rewrite).
Ensure non-recursive behavior is enforced, including appropriate error when a referenced
table does not exist.
Include documentation/validation for handling invalid JSON or SQL syntax within the hint.
⚪ Validate in the full engine that rewrite occurs at parse/plan time and not later, and that
no hidden recursion occurs beyond what tests cover.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Copy link

qodo-merge-pro bot commented Oct 23, 2025
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion Impact
High-level
Defer merging tests for non-working features

The PR includes many tests for complex queries that are marked as non-verifying
due to a dependency on an unresolved issue. It is recommended to either resolve
the issue first or use a standard mechanism to explicitly skip these tests.

Examples:

test/distributed/cases/hint/hint.sql [95-121]
-- @bvt:issue#22662
drop table if exists sales;
create table sales (
 sale_id int primary key,
 product_id int,
 quantity int,
 sale_date date
);
insert into sales values
 ... (clipped 17 lines)
test/distributed/cases/hint/hint.result [92-115]
[unknown result because it is related to issue#22662]
create table sales (
sale_id int primary key,
product_id int,
quantity int,
sale_date date
);
[unknown result because it is related to issue#22662]
insert into sales values
(1, 1, 10, '2025-01-01'),
 ... (clipped 14 lines)

Solution Walkthrough:

Before:

-- In hint.sql
-- @bvt:issue#22662
drop table if exists sales;
create table sales (...);
/*+ {
 "rewrites": {
 "hint_test.sales_summary": "SELECT product_id, SUM(quantity) ... FROM sales GROUP BY product_id"
 }
} */
select * from hint_test.sales_summary where total_quantity > 20;
-- In hint.result
...
[unknown result because it is related to issue#22662]
...

After:

-- Option 1: Remove the non-verifying tests for now.
-- (The test case from the 'before' snippet would be deleted from hint.sql and hint.result)
-- Option 2: Use a standard test skipping mechanism (if available).
-- @bvt:skip(issue="22662", reason="Complex query rewrites not yet supported")
drop table if exists sales;
create table sales (...);
/*+ {
 "rewrites": {
 "hint_test.sales_summary": "SELECT product_id, SUM(quantity) ... FROM sales GROUP BY product_id"
 }
} */
select * from hint_test.sales_summary where total_quantity > 20;
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that a significant portion of the new tests are non-verifying placeholders due to a known issue, which reduces the PR's value and adds maintenance debt.

Medium
Possible issue
(削除) Correct SQL syntax for count function (削除ここまで)
Suggestion Impact:The commit removed the space in the COUNT function, changing "select count (*) ..." to "select count(*) ...", albeit also changing the referenced table and surrounding context.

code diff:

-select count (*) from hint_test.large_table;
+/*+ {
+ "rewrites": {
+ "hint_test.large_table": "SELECT * FROM large_table WHERE id < 1000"
+ }
+} */
+select count(*) from large_table;
 select count(*) from large_table where id < 1000;

Correct the syntax error in the performance test by changing count () to
count(
). This will fix the parser error and allow the test to execute.

test/distributed/cases/hint/hint.sql [525-527]

 /*+ {"rewrites": {"hint_test.large_table": "SELECT * FROM large_table WHERE id < 1000"}} */
-select count (*) from hint_test.large_table;
+select count(*) from hint_test.large_table;
 select count(*) from large_table where id < 1000;

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a syntax error (count (*)) that causes the performance test to fail and prevents it from running, which is a critical issue for that test case.

Medium
Isolate case-sensitivity test from previous state

The case-sensitivity test is flawed due to reusing the users table name, leading
to ambiguous results. Use a unique table name like CaseTest to create an
isolated and reliable test.

test/distributed/cases/hint/hint.sql [466-474]

+drop table if exists users;
+create table CaseTest (
+ id int,
+ name varchar(50)
+);
+
+insert into CaseTest values (1, 'Test'), (2, 'Another');
 /*+ {
 "rewrites": {
- "hint_test.users": "SELECT * FROM Users WHERE id = 1",
- "hint_test.USERS": "SELECT * FROM Users WHERE id = 2"
+ "hint_test.casetest": "SELECT * FROM CaseTest WHERE id = 1",
+ "hint_test.CAETEST": "SELECT * FROM CaseTest WHERE id = 2"
 }
 } */
-SELECT * FROM users;
-SELECT * FROM USERS;
-SELECT * FROM Users;
+SELECT * FROM casetest;
+SELECT * FROM CAETEST;
+SELECT * FROM CaseTest;
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a flaw in the case-sensitivity test where results are ambiguous due to table name reuse, and proposes a valid fix to improve test isolation and clarity.

Medium
(削除) Fix invalid column references in query (削除ここまで)
Suggestion Impact:The committed patch defines the sales table with product_name and category columns before using them in the hint_test.top_products rewrite, aligning with the suggestion to fix invalid column references.

code diff:

--- subquery aggregation rewrite
-drop table if exists sales;
-create table sales (
- sale_id int primary key,
- product_id int,
- quantity int,
- sale_date date
-);
-insert into sales values
-(1, 1, 10, '2025-01-01'),
-(2, 1, 15, '2025-01-02'),
-(3, 2, 20, '2025-01-01'),
-(4, 2, 25, '2025-01-03'),
-(5, 3, 30, '2025-01-02');
-/*+ {
- "rewrites": {
- "hint_test.top_products": "SELECT product_id, product_name, category, (SELECT AVG(quantity) FROM sales s2 WHERE s2.category = s1.category) as category_avg_qty, quantity FROM sales s1 WHERE quantity > (SELECT AVG(quantity) FROM sales s3 WHERE s3.category = s1.category)"
- }
-} */
-select product_name, category, quantity, category_avg_qty
-from top_products
-order by category, quantity desc;
-drop table sales;

The rewrite hint for hint_test.top_products is invalid because it references
product_name and category columns which are not present in the sales table. Add
these columns to the sales table definition and corresponding data to fix the
test.

test/distributed/cases/hint/hint.sql [253-257]

+/*+ {
+ "rewrites": {
+ "hint_test.top_products": "SELECT product_id, product_name, category, (SELECT AVG(quantity) FROM sales s2 WHERE s2.category = s1.category) as category_avg_qty, quantity FROM sales s1 WHERE quantity > (SELECT AVG(quantity) FROM sales s3 WHERE s3.category = s1.category)"
+ }
+} */
 
-

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the test query references columns (product_name, category) that do not exist in the sales table definition, making the test case invalid.

Low
  • Update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@heni02 heni02 Awaiting requested review from heni02 heni02 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

kind/feature Review effort 3/5 size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /