feat(datasources): add query subcommand and reshape preview-query response#41
Conversation
…keep --raw for columnar
…es events test pattern)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/commands/datasources/datasources.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/commands/datasources/index.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/utils/stdin.ts (1)
34-43: ⚡ Quick winDeduplicate stdin consumption logic to prevent drift.
readStdinText()repeats the same stdin-read path already used byreadLinesFromStdin(). Consolidating to a single reader keeps behaviour changes in one place.♻️ Proposed refactor
export async function readLinesFromStdin(): Promise<string[]> { - if (!isStdinPiped()) { - console.error('Waiting for input on stdin... (pipe data or press Ctrl+D to end)'); - } - const chunks: Buffer[] = []; - for await (const chunk of process.stdin) { - chunks.push(chunk as Buffer); - } - return Buffer.concat(chunks) - .toString('utf-8') + const text = await readStdinText(); + return text .split('\n') .map((line) => line.trim()) .filter((line) => line.length > 0); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/utils/stdin.ts` around lines 34 - 43, The readStdinText() duplicates the stdin consumption logic already implemented in readLinesFromStdin(); refactor to deduplicate by extracting the common reader or delegating: either create a single helper (e.g., getStdinBuffer/getStdinChunks) that contains the for-await loop and returns a Buffer/string, and have both readStdinText() and readLinesFromStdin() call it, or implement readStdinText() by calling readLinesFromStdin() and joining lines into a single string; update references to readStdinText and readLinesFromStdin accordingly and ensure the initial "Waiting for input..." message remains consistent in the single shared reader.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/datasources/index.ts`:
- Around line 186-202: The code currently accepts --json-config but ignores the
parsed positional datasource id (variable id) allowing the JSON body to override
it; update the branch handling options.jsonConfig (where config =
validateJSON(...)) to enforce the positional id as authoritative by checking
config.datasource_id (or equivalent key) and either overwrite it with the
positional id (config.datasource_id = id) or fail-fast if the JSON value exists
and differs (throw or console.error + process.exit(1)); ensure you update the
same scope where validateJSON is called so downstream code uses the positional
id.
---
Nitpick comments:
In `@src/lib/utils/stdin.ts`:
- Around line 34-43: The readStdinText() duplicates the stdin consumption logic
already implemented in readLinesFromStdin(); refactor to deduplicate by
extracting the common reader or delegating: either create a single helper (e.g.,
getStdinBuffer/getStdinChunks) that contains the for-await loop and returns a
Buffer/string, and have both readStdinText() and readLinesFromStdin() call it,
or implement readStdinText() by calling readLinesFromStdin() and joining lines
into a single string; update references to readStdinText and readLinesFromStdin
accordingly and ensure the initial "Waiting for input..." message remains
consistent in the single shared reader.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7011b9c3-77de-4199-81d1-6616b2af1aee
📒 Files selected for processing (7)
README.mdsrc/commands/datasources/datasources.test.tssrc/commands/datasources/index.tssrc/core/datasources/datasources.tssrc/core/datasources/index.tssrc/lib/utils/stdin.test.tssrc/lib/utils/stdin.ts
| if (options.jsonConfig !== undefined) { | ||
| if ( | ||
| positionalSql !== undefined || | ||
| options.sql !== undefined || | ||
| options.limit !== undefined | ||
| ) { | ||
| console.error( | ||
| chalk.red( | ||
| '✗ --json-config cannot be combined with positional SQL, --sql, or --limit.' | ||
| ) | ||
| ); | ||
| process.exit(1); | ||
| } | ||
| config = validateJSON(options.jsonConfig, '--json-config') as Record< | ||
| string, | ||
| unknown | ||
| >; |
There was a problem hiding this comment.
Keep the positional <id> authoritative when --json-config is used.
This branch parses the required datasource ID and then ignores it. abs datasources query 6 --json-config '{"datasource_id":7,"query":"..."}' will run against datasource 7, which is very easy to miss in scripts. Please either inject datasource_id: id here or fail fast when the JSON body disagrees with the positional argument.
Suggested fix
- config = validateJSON(options.jsonConfig, '--json-config') as Record<
- string,
- unknown
- >;
+ const parsed = validateJSON(options.jsonConfig, '--json-config') as Record<
+ string,
+ unknown
+ >;
+ if (parsed.datasource_id !== undefined && parsed.datasource_id !== id) {
+ console.error(
+ chalk.red('✗ --json-config datasource_id must match the positional <id>.')
+ );
+ process.exit(1);
+ }
+ config = { ...parsed, datasource_id: id };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/datasources/index.ts` around lines 186 - 202, The code currently
accepts --json-config but ignores the parsed positional datasource id (variable
id) allowing the JSON body to override it; update the branch handling
options.jsonConfig (where config = validateJSON(...)) to enforce the positional
id as authoritative by checking config.datasource_id (or equivalent key) and
either overwrite it with the positional id (config.datasource_id = id) or
fail-fast if the JSON value exists and differs (throw or console.error +
process.exit(1)); ensure you update the same scope where validateJSON is called
so downstream code uses the positional id.
|
Actionable comments posted: 0 |
1 participant
Summary
abs datasources query <id> [sql]subcommand for ad-hoc SQL: takes the datasource id positionally and SQL either positionally, via--sql <text>, or via--sql -(stdin). Optional--limit <n>.--json-configretained as an escape hatch.queryandpreview-querynow reshape the columnar API response ({ columnNames, columnTypes, rows }) into row-objects via the existingcolumnarToRowshelper, so-o table,-o vertical,-o json, etc. render naturally. Same conventionevents listalready uses.--rawpreserves the original columnar payload on both commands.Before
After
Behavior change
preview-query's default output shape flips from{ columnNames, columnTypes, rows }toArray<Record<string, unknown>>. Scripts parsingrows[i][j]will break; pass--rawto restore the previous shape exactly.Test plan
--sql/--limit/--json-config, every mutual-exclusion error path, stdin reading + empty-stdin rejection, reshape default,--rawpassthrough — for bothqueryandpreview-query.npm run typecheckcleannpm run lintcleannpm run test:run— 2465 passed / 4 skipped / 0 faileddev-1(datasource 6, BigQuery): positional +--limit→ table;-o vertical→ per-row transpose;--raw -o yaml→ columnar payload preserved;echo "SELECT 1" | abs ds query 6 --sql -→ table; validation errors exit 1 with clear messages.Out of scope
validate-query,introspect,test,schema, or any other datasources subcommand.--limitis the only knob.Summary by CodeRabbit
querysubcommand with inline SQL,--sql(including--sql -to read from stdin),--limit,--json-configand a--rawoption to preserve columnar output; default reshapes to row objects.--raw.--rawvs reshaped output.