-
Notifications
You must be signed in to change notification settings - Fork 11
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
Added a new detector for catching unsupported COPY command structure #2106
Changes from all commits
c7e1930
038fd11
1069f60
e387953
f70de4d
be6ca71
e687c7d
a417d93
3a6915f
5eace08
06ab585
f22d7c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -649,3 +649,42 @@ func TestRegexFunctionsIssue(t *testing.T) { | |
} | ||
|
||
} | ||
|
||
func TestCopyUnsupportedConstructIssuesDetected(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see if the tests for this can be at one place only either in detectors.go or parse_issue_detector_test.go Currently we have the tests distributed among these two files, but since they are essentially testing the same thing, we can just have it one place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah for now I have added the tests but commented out the sql of CREATE ... ON_ERROR since it is supported only on PG17. Have added a ticket to do the same: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the tests from detectors_test.go. The tests are same anyways. They were helpful in development. |
||
expectedIssues := map[string][]QueryIssue{ | ||
`COPY my_table FROM '/path/to/data.csv' WHERE col1 > 100;`: {NewCopyFromWhereIssue("DML_QUERY", "", `COPY my_table FROM '/path/to/data.csv' WHERE col1 > 100;`)}, | ||
`COPY my_table(col1, col2) FROM '/path/to/data.csv' WHERE col2 = 'test';`: {NewCopyFromWhereIssue("DML_QUERY", "", `COPY my_table(col1, col2) FROM '/path/to/data.csv' WHERE col2 = 'test';`)}, | ||
`COPY my_table FROM '/path/to/data.csv' WHERE TRUE;`: {NewCopyFromWhereIssue("DML_QUERY", "", `COPY my_table FROM '/path/to/data.csv' WHERE TRUE;`)}, | ||
`COPY employees (id, name, age) | ||
FROM STDIN WITH (FORMAT csv) | ||
WHERE age > 30;`: {NewCopyFromWhereIssue("DML_QUERY", "", `COPY employees (id, name, age) | ||
FROM STDIN WITH (FORMAT csv) | ||
WHERE age > 30;`)}, | ||
|
||
`COPY table_name (name, age) FROM '/path/to/data.csv' WITH (FORMAT csv, HEADER true, ON_ERROR IGNORE);`: {NewCopyOnErrorIssue("DML_QUERY", "", `COPY table_name (name, age) FROM '/path/to/data.csv' WITH (FORMAT csv, HEADER true, ON_ERROR IGNORE);`)}, | ||
`COPY table_name (name, age) FROM '/path/to/data.csv' WITH (FORMAT csv, HEADER true, ON_ERROR STOP);`: {NewCopyOnErrorIssue("DML_QUERY", "", `COPY table_name (name, age) FROM '/path/to/data.csv' WITH (FORMAT csv, HEADER true, ON_ERROR STOP);`)}, | ||
|
||
`COPY table_name (name, age) FROM '/path/to/data.csv' WITH (FORMAT csv, HEADER true, ON_ERROR IGNORE) WHERE age > 18;`: {NewCopyFromWhereIssue("DML_QUERY", "", `COPY table_name (name, age) FROM '/path/to/data.csv' WITH (FORMAT csv, HEADER true, ON_ERROR IGNORE) WHERE age > 18;`), NewCopyOnErrorIssue("DML_QUERY", "", `COPY table_name (name, age) FROM '/path/to/data.csv' WITH (FORMAT csv, HEADER true, ON_ERROR IGNORE) WHERE age > 18;`)}, | ||
`COPY table_name (name, age) FROM '/path/to/data.csv' WITH (FORMAT csv, HEADER true, ON_ERROR STOP) WHERE name = 'Alice';`: {NewCopyFromWhereIssue("DML_QUERY", "", `COPY table_name (name, age) FROM '/path/to/data.csv' WITH (FORMAT csv, HEADER true, ON_ERROR STOP) WHERE name = 'Alice';`), NewCopyOnErrorIssue("DML_QUERY", "", `COPY table_name (name, age) FROM '/path/to/data.csv' WITH (FORMAT csv, HEADER true, ON_ERROR STOP) WHERE name = 'Alice';`)}, | ||
|
||
`COPY my_table FROM '/path/to/data.csv' WITH (FORMAT csv);`: {}, | ||
`COPY my_table FROM '/path/to/data.csv' WITH (FORMAT text);`: {}, | ||
`COPY my_table FROM '/path/to/data.csv';`: {}, | ||
`COPY my_table FROM '/path/to/data.csv' WITH (DELIMITER ',');`: {}, | ||
`COPY my_table(col1, col2) FROM '/path/to/data.csv' WITH (FORMAT csv, HEADER true);`: {}, | ||
} | ||
|
||
parserIssueDetector := NewParserIssueDetector() | ||
|
||
for stmt, expectedIssues := range expectedIssues { | ||
issues, err := parserIssueDetector.getDMLIssues(stmt) | ||
fatalIfError(t, err) | ||
assert.Equal(t, len(expectedIssues), len(issues)) | ||
for _, expectedIssue := range expectedIssues { | ||
found := slices.ContainsFunc(issues, func(queryIssue QueryIssue) bool { | ||
return cmp.Equal(expectedIssue, queryIssue) | ||
}) | ||
assert.True(t, found, "Expected issue not found: %v in statement: %s", expectedIssue, stmt) | ||
} | ||
} | ||
} |
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.
Simplified code from what I was discussing in the morning:
I tried running your tests and they all passed, but do take a deeper look!
cc: @sanyamsinghal @priyanshi-yb
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.
I think yes this makes sense. It reduces the number of lines and readable.
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.
Lets stick to protomsg for this codepath, reason being the required info is not directly availabe in the struct and extraction of defElem can be generic and reused multiple times.
It can be simplified like this using existing helper functions:
Refer my PR for
TraverseAndExtractDefNamesFromDefElem
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.
just to be on the same page, there are two different issues here.
For the first one, isFrom and whereClause are directly available in
CopyStmt
struct, so would prefer dealing with the struct directly for that issue. ButGetMessageField(msg, "is_from"
isn't too bad either from a readability standpoint. I'll leave it up to you.For the DefElem one, we can use the new function that Sanyam is introducing, but then you'll have to wait until that PR is merged.
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.
I had to add a new function GetBoolField() for the is_from clause. I have used the GetMessageField and GetBoolField approach rn for the first one.
For the second one, I have used Sanyam's function in my PR.