Conversation
(cherry picked from commit 3b7155d)
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
release-note
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds parsing and AST support for the SQL variants prefixed by RAW: Changes
Sequence Diagram(s)sequenceDiagram
participant Client as SQL Input
participant Lexer
participant Parser
participant AST as AST Builder
Client->>Lexer: "SHOW RAW IMPORT JOBS"
Lexer->>Parser: tokens: SHOW, RAW, IMPORT, JOBS
Parser->>Parser: match ShowImportJobsTarget (RAW IMPORT JOBS)
Parser->>AST: construct ShowStmt{Tp: ShowImportJobs, ImportJobRaw: true}
AST-->>Client: ShowStmt(ImportJobRaw=true)
Client->>Lexer: "SHOW RAW IMPORT JOB 123 WHERE group_key='g'"
Lexer->>Parser: tokens: SHOW, RAW, IMPORT, JOB, 123, WHERE, IDENT, '=', STRING
Parser->>Parser: match ShowImportJobTarget + Int64Num + filter
Parser->>AST: construct ShowStmt{Tp: ShowImportJobs, ImportJobID: 123, ImportJobRaw: true, Filter: ...}
AST-->>Client: ShowStmt(ImportJobID=123, ImportJobRaw=true, Filter=...)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
bot
added
the
size/XXL
|
Hi @GMHDBJD. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/parser/ast/dml.go (1)
3434-3443:⚠️ Potential issue | 🟠 MajorPropagate
restoreShowLikeOrWhereOpt()errors in theSHOW ... IMPORT JOBSrestore path.At Line 3442, the returned error is ignored. This can silently mask restore failures for both RAW and non-RAW
SHOW IMPORT JOBS.Suggested fix
} else { ctx.WriteKeyWord("IMPORT JOBS") - restoreShowLikeOrWhereOpt() + if err := restoreShowLikeOrWhereOpt(); err != nil { + return err + } }As per coding guidelines, keep error handling actionable and contextual; avoid silently swallowing errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/ast/dml.go` around lines 3434 - 3443, The SHOW IMPORT JOBS restore path currently calls restoreShowLikeOrWhereOpt() and ignores its error; update the call site so you capture the returned error from restoreShowLikeOrWhereOpt(), and if non-nil return (or wrap with contextual message) up the call stack instead of discarding it; ensure the surrounding function (the restore/format method that contains the ctx.WriteKeyWord("IMPORT JOBS") logic) propagates the error to the caller so restore failures for RAW and non-RAW paths aren't silently swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/parser/ast/dml.go`:
- Around line 3434-3443: The SHOW IMPORT JOBS restore path currently calls
restoreShowLikeOrWhereOpt() and ignores its error; update the call site so you
capture the returned error from restoreShowLikeOrWhereOpt(), and if non-nil
return (or wrap with contextual message) up the call stack instead of discarding
it; ensure the surrounding function (the restore/format method that contains the
ctx.WriteKeyWord("IMPORT JOBS") logic) propagates the error to the caller so
restore failures for RAW and non-RAW paths aren't silently swallowed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 26dad9b9-101e-4c0c-b29c-8353c70e3505
📒 Files selected for processing (7)
pkg/parser/ast/dml.gopkg/parser/ast/dml_test.gopkg/parser/keywords.gopkg/parser/keywords_test.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.y
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67521 +/- ##
================================================
- Coverage 77.5921% 77.4063% -0.1859%
================================================
Files 1981 1965 -16
Lines 548142 548618 +476
================================================
- Hits 425315 424665 -650
- Misses 122017 123951 +1934
+ Partials 810 2 -808
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
🔍 Starting code review for this PR... |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 1
- Inline comments: 1
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (1)
ImportJobRawflag silently dropped at plan-build time — SHOW RAW behaves as SHOW (pkg/parser/ast/dml.go:3209, pkg/planner/core/planbuilder.go:3498, pkg/executor/builder.go:930, pkg/planner/core/operator/logicalop/logical_show.go:61)
|
🔍 Processing follow-up on review comments... |
pkg/parser/parser.y
Outdated
| ImportJobID: &v, | ||
| } | ||
| } | ||
| | "SHOW" "RAW" "IMPORT" "JOB" Int64Num |
There was a problem hiding this comment.
can we merge the rule with above, so we can decode RAW as optional
below "RAW" "IMPORT" "JOBS" too
There was a problem hiding this comment.
Refactored this into helper nonterminals in parser.y so both SHOW RAW IMPORT JOB <id> and SHOW RAW IMPORT JOBS reuse the same optional-RAW structure without introducing parser conflicts. Updated in 0aac5bb.
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
1 similar comment
|
/retest |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, lance6716, yudongusa The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
# Conflicts: # pkg/parser/parser.go
|
/retest |
|
@GMHDBJD: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
/ok-to-test |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
6 participants
What problem does this PR solve?
Issue Number: ref #66461
Problem Summary:
This PR extracts the parser-related part from #66502 so the syntax and AST changes for
SHOW RAW IMPORT JOB(S)can be reviewed separately.What changed and how does it work?
SHOW RAW IMPORT JOB <job_id>andSHOW RAW IMPORT JOBS.ImportJobRawtoast.ShowStmtand restore theRAWprefix in SQL text.RAW.master.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Tests