tests(select_exclude) + sql(select_exclude): implement SELECT ... EXCLUDE(...) and normalize mysql-test files#647
Open
hlosukwakha wants to merge 3 commits intomysql:trunkfrom
Open
tests(select_exclude) + sql(select_exclude): implement SELECT ... EXCLUDE(...) and normalize mysql-test files#647hlosukwakha wants to merge 3 commits intomysql:trunkfrom
SELECT ... EXCLUDE(...) and normalize mysql-test files#647hlosukwakha wants to merge 3 commits intomysql:trunkfrom
Conversation
… wildcard expansion)
```markdown # Title **tests(select_exclude) + sql(select_exclude): implement `SELECT ... EXCLUDE(...)` and normalize mysql-test files** --- ## Summary Implements the SQL extension `SELECT ... EXCLUDE(...)` end-to-end and normalizes the mysql-test files required to validate the feature. - Adds `EXCLUDE` grammar and parser support. - Attaches exclude-list metadata to the AST `Item_asterisk`. - Propagates exclude-lists through the resolver into wildcard expansion. - Normalizes/cleans MTR test files so the local harness passes (removes code fences, fixes spacing/newlines, aligns `.result` output with harness logs). - This MR contains both the source changes and the corresponding test-suite updates necessary to validate the feature locally. --- ## What changed (high level) ### Source - **Parser** - `sql_yacc.yy` — new `EXCLUDE` production to parse `EXCLUDE (ident_list)` and build parser-allocated lists. - **AST / Items** - `item.h` — `Item_asterisk` extended to hold an exclude-list pointer. - **Wildcard expansion / helpers** - `sql_base.h` / `sql_base.cc` — `insert_fields` updated to accept and apply exclude lists when expanding `*`. - **Resolver** - `sql_resolver.cc` — `Query_block::setup_wild` reads `Item_asterisk::m_exclude_list` and forwards it to the expansion routine. - Minor supporting changes to show/processlist files (to avoid accidental regression while adding the feature). ### Tests - `select_exclude_multi_table.test` and `.result` — multi-table tests for `EXCLUDE(...)` (cleaned of formatting artifacts and normalized). - `select_exclude_edgecases.test` and `.result` — edge-case tests (duplicates, excluding all columns, nested selects); cleaned and normalized. - (See full file list below.) --- ## Files changed (representative) - `sql_yacc.yy` - `item.h` - `sql_base.h` - `sql_base.cc` - `sql_resolver.cc` - `show_query_builder.cc` - `sql_show_processlist.cc` - `sql_show_status.cc` - `select_exclude_multi_table.test` - `select_exclude_multi_table.result` - `select_exclude_edgecases.test` - `select_exclude_edgecases.result` --- ## Commits - `f1d5595f92b` — **tests(select_exclude):** normalize test and `.result` files for select_exclude feature - `e7d7709dc4c` — **sql(select_exclude):** implement `EXCLUDE` syntax (parser, `Item_asterisk`, wildcard expansion) --- ## Why this change The `EXCLUDE(...)` syntax provides a convenient way to select all columns except a small list of columns (PII, secret fields, etc.) without enumerating the full column list. - Adds end-to-end support (parse → AST → resolve → expand) and tests to validate behavior. --- ## Backwards compatibility / risk - This introduces a new SQL grammar construct. It is additive and does not change existing syntax. - Minimal risk to existing features if the exclude list is simply ignored for contexts that don't use it. - Tests were normalized, and I used the exact harness output to ensure MTR compatibility — please double-check CI environments for any locale/ICU differences. --- ## How to test locally (quick) From the repository root: 1. Build/run your usual out-of-source CMake build as you normally would (if you need to run a full server). 2. Run just the affected MTR tests (fast local check). Expected: the three tests pass locally (they passed for me after normalizing the tests). Inspect logs if needed: - `mysql-test/var/log/local.select_exclude_multi_table/` - `mysql-test/var/log/local.select_exclude_edgecases/` --- ## CI considerations The test normalization was done to match the local harness. CI may run in a slightly different environment (ICU/data directories, locale, or tab/space rendering). If CI shows mismatches: - Confirm the CI runtime and mysql-test environment (ICU data paths or locale). - If message wording differs on CI, update `.result` to the canonical CI output or make the test less strict where appropriate. - Consider adding the new local tests to the CI collection that runs for feature branches. --- ## Notes / implementation details - **Parser:** new `opt_exclude` production returns a `List<String> *` allocated by the parser; this is attached to `Item_asterisk`. - **AST:** `Item_asterisk` stores `m_exclude_list` and forwards it during resolve. - `insert_fields(...)` now accepts an exclude-list pointer and filters column names before appending them to the select list. - **Resolver:** `Query_block::setup_wild` detects `Item_asterisk` and forwards the exclude list to `insert_fields`. --- ## Review checklist - Parser changes: scanning and bison-generated output reviewed for correctness. - `Item_asterisk` changes: ensure memory ownership expectations are respected when parser-allocated lists are attached to AST nodes. - `insert_fields` changes: test with qualified/unqualified stars, duplicate names, and multi-table joins. - Run impacted unit tests and MTR suite subset. - Confirm there are no unexpected changes to `mysqld` error or logging wording. --- ## Suggested reviewers - SQL parser / grammar maintainers - SQL resolver / query-expansion owners - MTR/test-maintainers for the mysql-test changes --- ## Additional follow-ups (optional) Expand MTR coverage to cover: - Qualified vs unqualified stars in complex join/CTE scenarios. - Interaction with `SELECT ... INTO` and `INSERT ... SELECT`. - Behavior when exclusion lists include columns from multiple source tables with the same name. Other: - Add inline comments to new parser/AST fields to describe memory ownership and lifetime expectations. - Consider adding integration tests that run under CI's environment to avoid environment-specific `.result` mismatches (locale/ICU). --- ```
|
Hi, thank you for submitting this pull request. In order to consider your code we need you to sign the Oracle Contribution Agreement (OCA). Please review the details and follow the instructions at https://oca.opensource.oracle.com/ |
This file contains hidden or 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
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.
Summary
Implements the SQL extension
SELECT ... EXCLUDE(...)end-to-end and normalizes the mysql-test files required to validate the feature.EXCLUDEgrammar and parser support.Item_asterisk..resultoutput with harness logs).What changed (high level)
Source
sql_yacc.yy— newEXCLUDEproduction to parseEXCLUDE (ident_list)and build parser-allocated lists.item.h—Item_asteriskextended to hold an exclude-list pointer.sql_base.h/sql_base.cc—insert_fieldsupdated to accept and apply exclude lists when expanding*.sql_resolver.cc—Query_block::setup_wildreadsItem_asterisk::m_exclude_listand forwards it to the expansion routine.Tests
select_exclude_multi_table.testand.result— multi-table tests forEXCLUDE(...)(cleaned of formatting artifacts and normalized).select_exclude_edgecases.testand.result— edge-case tests (duplicates, excluding all columns, nested selects); cleaned and normalized.Files changed (representative)
sql_yacc.yyitem.hsql_base.hsql_base.ccsql_resolver.ccshow_query_builder.ccsql_show_processlist.ccsql_show_status.ccselect_exclude_multi_table.testselect_exclude_multi_table.resultselect_exclude_edgecases.testselect_exclude_edgecases.resultCommits
f1d5595f92b— tests(select_exclude): normalize test and.resultfiles for select_exclude featuree7d7709dc4c— sql(select_exclude): implementEXCLUDEsyntax (parser,Item_asterisk, wildcard expansion)Why this change
The
EXCLUDE(...)syntax provides a convenient way to select all columns except a small list of columns (PII, secret fields, etc.) without enumerating the full column list.Backwards compatibility / risk
How to test locally (quick)
From the repository root:
Expected: the three tests pass locally (they passed for me after normalizing the tests). Inspect logs if needed:
mysql-test/var/log/local.select_exclude_multi_table/mysql-test/var/log/local.select_exclude_edgecases/CI considerations
The test normalization was done to match the local harness. CI may run in a slightly different environment (ICU/data directories, locale, or tab/space rendering). If CI shows mismatches:
.resultto the canonical CI output or make the test less strict where appropriate.Notes / implementation details
opt_excludeproduction returns aList<String> *allocated by the parser; this is attached toItem_asterisk.Item_asteriskstoresm_exclude_listand forwards it during resolve.insert_fields(...)now accepts an exclude-list pointer and filters column names before appending them to the select list.Query_block::setup_wilddetectsItem_asteriskand forwards the exclude list toinsert_fields.Review checklist
Item_asteriskchanges: ensure memory ownership expectations are respected when parser-allocated lists are attached to AST nodes.insert_fieldschanges: test with qualified/unqualified stars, duplicate names, and multi-table joins.mysqlderror or logging wording.Suggested reviewers
Additional follow-ups (optional)
Expand MTR coverage to cover:
SELECT ... INTOandINSERT ... SELECT.Other:
.resultmismatches (locale/ICU).