fix: load paths, imports, transpile, and CI permissions#2128
fix: load paths, imports, transpile, and CI permissions#2128Chessing234 wants to merge 1 commit into
Conversation
Quote/escape datadirs and passwords in loaders, accept plain CSV in duckdb/sqlite imports, utf-8 transpile writes, and don't fail sqlfluff annotate on fork PRs. Co-authored-by: Cursor <cursoragent@cursor.com>
alistairewj
left a comment
alistairewj
left a comment
There was a problem hiding this comment.
the changes are scattered and not very cohesive but with some cleaning up we can merge
| run: sqlfluff lint --format github-annotation --annotation-level failure --nofail ${{ steps.get_files_to_lint.outputs.lintees }} > annotations.json | ||
| - name: Annotate | ||
| # Fork PRs cannot create check runs with GITHUB_TOKEN; lint already ran. | ||
| continue-on-error: true |
There was a problem hiding this comment.
too blunt; remove it, add workspace permission
| USAGE: ./import_duckdb.sh mimic_data_dir [output_db] | ||
| WHERE: | ||
| mimic_data_dir directory that contains csv.gz or csv files | ||
| mimic_data_dir directory that contains csv.tar.gz or csv files |
There was a problem hiding this comment.
we don't distribute .tar.gz files
|
|
||
|
|
||
| # GENERATE_ARRAY(a, b) -> list via generate_series (inclusive), for UNNEST. | ||
| def _generate_array_sql(self: DuckDB.Generator, expression: exp.Expression) -> str: |
There was a problem hiding this comment.
this change is not necessary as it all transpiles correctly using the pinned sqlglot, unnest works fine in my testing on duckdb
| @echo '' | ||
| @sleep 2 | ||
| cd ../../concepts_postgres/ && psql "$(DBSTRING)" -v ON_ERROR_STOP=1 -c "CREATE SCHEMA IF NOT EXISTS mimiciii_derived;" -f postgres-make-concepts.sql | ||
| cd ../../concepts/ && psql "$(DBSTRING)" -v ON_ERROR_STOP=1 -f make-concepts.sql |
| fn_stem=`echo $fn | cut -d. -f 1` | ||
| for fn_path in "$REPORT_PATH"/*.csv; do | ||
| [ -f "$fn_path" ] || continue | ||
| fn=$(basename "$fn_path") |
There was a problem hiding this comment.
there are a lot of these excessive changes - none of the files have dots in their name, this fix is unnecessarily defensive, there are bigger issues to spend effort on please!
| title: "SQLFluff Lint" | ||
| input: "./annotations.json" No newline at end of file | ||
| input: "./annotations.json" | ||
| ignore-missing-file: true No newline at end of file |
There was a problem hiding this comment.
this should be ignore-unauthorized-error: true
| echo $OUTPUT_DIR - running NegBio.. | ||
| python $NEGBIO_PATH/negbio/negbio_csv2bioc.py --output $OUTPUT_DIR/report $INPUT_FILE | ||
| echo "$OUTPUT_DIR - running NegBio.." | ||
| python "$NEGBIO_PATH/negbio/negbio_csv2bioc.py" --output "$OUTPUT_DIR/report" "$INPUT_FILE" |
There was a problem hiding this comment.
why fix quoting this and not fix the rest of the unquoted paths?
again - another example of very defensive changes that don't actually fix a real bug. not super interested in these in general, they take time away from bigger impact changes
| # Column dtypes for tables that trigger pandas mixed-type warnings when | ||
| # loaded with the default low_memory chunked inference (see #1237). | ||
| # Types mirror mimic-iii/buildmimic/postgres/postgres_create_tables.sql. | ||
| TABLE_DTYPES = { |
There was a problem hiding this comment.
this is great but can you split it into its own PR and add CI that verifies it?
| [ -z "${tbl}" ] && continue | ||
| if ! grep -q "^${tbl}," <<< "${ACTUAL}"; then | ||
| # exact field match (regex grep can false-hit prefixes) | ||
| if ! awk -F, -v t="${tbl}" '$1 == t { found=1 } END { exit !found }' <<< "${ACTUAL}"; then |
There was a problem hiding this comment.
again (I think I made this comment on an earlier PR) it can't because false-hit because of the prefix/trailing comma and we never have regex chars in table names. the comment is wrong and the change is unnecessary
2 participants
Summary
.csv(and.csv.gz) in duckdb/sqlite imports; utf-8 transpile writesTest plan
pytest tests/test_transpile.pyMade with Cursor