From 06996036d6065a3b4517dcb77138cdd8ace8a885 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Mon, 28 Sep 2026 13:40:54 +0200 Subject: [PATCH 1/2] Bump and functionality --- CHANGELOG.md | 9 ++ doc/get_started/tooling/positron-vscode.qmd | 6 + ggsql-vscode/.gitignore | 3 + ggsql-vscode/package-lock.json | 8 +- ggsql-vscode/package.json | 5 +- ggsql-vscode/scripts/sync-skill.js | 34 ++++ ggsql-vscode/src/dataImporter.ts | 166 ++++++++++++++++++++ ggsql-vscode/src/extension.ts | 24 +++ ggsql-vscode/src/test/dataImporter.test.ts | 155 ++++++++++++++++++ 9 files changed, 404 insertions(+), 6 deletions(-) create mode 100644 ggsql-vscode/scripts/sync-skill.js create mode 100644 ggsql-vscode/src/dataImporter.ts create mode 100644 ggsql-vscode/src/test/dataImporter.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 0672bc4e2..53e92ca60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ ## [Unreleased] +### Added + +- The Positron extension now registers a ggsql data importer, so dragging a + csv/tsv/parquet/json file into Positron offers to generate the ggsql code + that loads it into a table, including any filters and sorts shown in the + Data Explorer (#536). +- The Positron extension now registers a bundled agent skill, so agents + automatically discover how to write and run ggsql queries (#536). + ### Fixed - Fixed a parser bug that interpreted comment characters inside string literals diff --git a/doc/get_started/tooling/positron-vscode.qmd b/doc/get_started/tooling/positron-vscode.qmd index de213c349..34af05621 100644 --- a/doc/get_started/tooling/positron-vscode.qmd +++ b/doc/get_started/tooling/positron-vscode.qmd @@ -61,6 +61,12 @@ If you use `.sql` files with other database tooling and would prefer the extensi "ggsql.enableSqlFiles": false ``` +## Importing data files + +Dragging a `.csv`, `.tsv`, `.parquet`, or `.json` file into Positron offers to generate the code that loads it, and ggsql is one of the importers on offer. The generated code is a `CREATE TABLE … AS SELECT …` statement that reads the file into a table, and if you open the import dialog from a Data Explorer view it can also reproduce the filters and sorts you have applied there as `WHERE` and `ORDER BY` clauses. + +The generated code targets a duckdb session — the default, empty in-memory connection ggsql starts with. Reading files with `FROM 'file.csv'` is duckdb functionality, so the import will not run in a session attached to another database backend with `-- @connect:`. For the same reason, filter expressions use duckdb syntax such as `ILIKE` and `regexp_matches()`; anything the importer cannot translate is listed as a warning next to the generated code rather than dropped silently. + ## Database connections By default, ggsql starts in the Positron console with an empty in-memory duckdb database connection. A "magic" comment can be used to initiate a different database connection after the session has launched, which can be either a comment in your source file or invoked directly in the console. diff --git a/ggsql-vscode/.gitignore b/ggsql-vscode/.gitignore index d8d1593a6..8bc5ef5cb 100644 --- a/ggsql-vscode/.gitignore +++ b/ggsql-vscode/.gitignore @@ -4,3 +4,6 @@ out-test .positron-test/ bundled *.vsix + +# Generated at package time by scripts/sync-skill.js from doc/vendor/SKILL.md +skills/ggsql/SKILL.md diff --git a/ggsql-vscode/package-lock.json b/ggsql-vscode/package-lock.json index 3319f5dee..62ce2a22e 100644 --- a/ggsql-vscode/package-lock.json +++ b/ggsql-vscode/package-lock.json @@ -12,7 +12,7 @@ "toml": "^3.0.0" }, "devDependencies": { - "@posit-dev/positron": "^0.2.7", + "@posit-dev/positron": "^0.2.9", "@posit-dev/positron-test-electron": "^0.0.3", "@types/mocha": "^10.0.10", "@types/node": "^18.x", @@ -754,9 +754,9 @@ } }, "node_modules/@posit-dev/positron": { - "version": "0.2.7", - "resolved": "https://registry.npmjs.org/@posit-dev/positron/-/positron-0.2.7.tgz", - "integrity": "sha512-BNTWBi3IshSbtbVS7WdgbhGeHCv2NNpiWjB+ovll6GcdLCOtu5MTeAxPwddCZOg7xSSKwq8s+l4f+5ANPExBFw==", + "version": "0.2.10", + "resolved": "https://registry.npmjs.org/@posit-dev/positron/-/positron-0.2.10.tgz", + "integrity": "sha512-CxJg5cTOy7QPt9FjO9m0NHbFBRQnlN+APW+Zr3j/7AWGe7RiDy19xLugfpyR8kyKZWsS9gFGeFjnqfL1zQHzow==", "dev": true, "license": "MIT", "engines": { diff --git a/ggsql-vscode/package.json b/ggsql-vscode/package.json index 699e2cb57..4384361fe 100644 --- a/ggsql-vscode/package.json +++ b/ggsql-vscode/package.json @@ -178,7 +178,8 @@ "watch": "npm-run-all -p watch:*", "watch:esbuild": "node esbuild.js --watch", "watch:tsc": "tsc --noEmit --watch --project tsconfig.json", - "package": "npm run check-types && node esbuild.js --production", + "sync-skill": "node scripts/sync-skill.js", + "package": "npm run sync-skill && npm run check-types && node esbuild.js --production", "check-types": "tsc --noEmit", "lint": "eslint src --ext ts", "compile-tests": "tsc -p tsconfig.test.json", @@ -192,7 +193,7 @@ "toml": "^3.0.0" }, "devDependencies": { - "@posit-dev/positron": "^0.2.7", + "@posit-dev/positron": "^0.2.9", "@posit-dev/positron-test-electron": "^0.0.3", "@types/mocha": "^10.0.10", "@types/node": "^18.x", diff --git a/ggsql-vscode/scripts/sync-skill.js b/ggsql-vscode/scripts/sync-skill.js new file mode 100644 index 000000000..3f8b2e9aa --- /dev/null +++ b/ggsql-vscode/scripts/sync-skill.js @@ -0,0 +1,34 @@ +/* + * Copies the canonical ggsql agent skill into the extension before packaging. + * + * The canonical source within this repo is doc/vendor/SKILL.md, which + * ggsql-cli/build.rs keeps in sync with the posit-dev/skills repository + * (rebuild the CLI with GGSQL_UPDATE_SKILL=1 to refresh it). The extension + * registers the skills/ directory as an agent skill root, so the packaged + * copy must live inside the extension; this script materialises it at + * package time rather than keeping a second, hand-maintained copy that + * would drift. + */ + +const fs = require('fs'); +const path = require('path'); + +const repoRoot = path.join(__dirname, '..', '..'); +const source = path.join(repoRoot, 'doc', 'vendor', 'SKILL.md'); +const destDir = path.join(__dirname, '..', 'skills', 'ggsql'); +const dest = path.join(destDir, 'SKILL.md'); + +const content = fs.readFileSync(source, 'utf8'); + +// Positron discovers skills by name/description frontmatter; fail loudly if +// the canonical file ever loses them instead of shipping a broken skill. +for (const field of ['name:', 'description:']) { + if (!content.startsWith('---') || !content.includes(`\n${field}`)) { + console.error(`sync-skill: ${source} is missing '${field}' frontmatter`); + process.exit(1); + } +} + +fs.mkdirSync(destDir, { recursive: true }); +fs.writeFileSync(dest, content); +console.log(`sync-skill: copied ${path.relative(repoRoot, source)} -> ${path.relative(repoRoot, dest)}`); diff --git a/ggsql-vscode/src/dataImporter.ts b/ggsql-vscode/src/dataImporter.ts new file mode 100644 index 000000000..02a9bf4bc --- /dev/null +++ b/ggsql-vscode/src/dataImporter.ts @@ -0,0 +1,166 @@ +/* + * ggsql data importer. + * + * Registers a Data Explorer importer so that dragging a csv/parquet/json file + * into Positron offers "ggsql" as a way to load it. The generated code is a + * ggsql query that reads the file into a table, reproducing any row filters + * and sorts from the current Data Explorer view. + */ + +import * as positron from '@posit-dev/positron'; + +/** File extensions ggsql can read through its duckdb backend. */ +const READABLE_EXTENSIONS = ['csv', 'tsv', 'parquet', 'json', 'jsonl', 'ndjson']; + +/** Column types whose stringified values can be embedded in SQL unquoted. */ +const NUMERIC_TYPES = new Set(['integer', 'number', 'float', 'double', 'decimal']); + +/** A small list of SQL reserved words, so Positron can suffix colliding variable names. */ +const RESERVED_NAMES = [ + 'select', 'from', 'where', 'table', 'group', 'order', 'by', 'insert', + 'update', 'delete', 'create', 'drop', 'join', 'union', 'all', 'and', + 'or', 'not', 'null', 'as', 'on', 'in', 'between', 'like', 'limit', +]; + +/** Quote a string value for SQL, escaping embedded quotes by doubling. */ +function quoteString(value: string): string { + return `'${value.replace(/'/g, "''")}'`; +} + +/** Quote an identifier (column or table name) for SQL. */ +function quoteIdentifier(name: string): string { + return `"${name.replace(/"/g, '""')}"`; +} + +/** + * Render a stringified filter value as a SQL literal, using the column's + * display type to decide whether quoting is needed. + */ +function renderValue(value: string, columnType: string): string { + const type = columnType.toLowerCase(); + if (NUMERIC_TYPES.has(type)) { + return value; + } + if (type === 'boolean') { + return value.toLowerCase() === 'true' ? 'TRUE' : 'FALSE'; + } + return quoteString(value); +} + +/** Translate one Data Explorer row filter into a SQL predicate. */ +function renderFilter(filter: positron.DataImportRowFilter): string | undefined { + const column = quoteIdentifier(filter.columnName); + switch (filter.filterType) { + case 'between': + return `${column} BETWEEN ${renderValue(filter.leftValue, filter.columnType)} AND ${renderValue(filter.rightValue, filter.columnType)}`; + case 'not_between': + return `${column} NOT BETWEEN ${renderValue(filter.leftValue, filter.columnType)} AND ${renderValue(filter.rightValue, filter.columnType)}`; + case 'compare': + return `${column} ${filter.op} ${renderValue(filter.value, filter.columnType)}`; + case 'search': { + const like = filter.caseSensitive ? 'LIKE' : 'ILIKE'; + switch (filter.searchType) { + case 'contains': + return `${column} ${like} ${quoteString(`%${filter.term}%`)}`; + case 'not_contains': + return `${column} NOT ${like} ${quoteString(`%${filter.term}%`)}`; + case 'starts_with': + return `${column} ${like} ${quoteString(`${filter.term}%`)}`; + case 'ends_with': + return `${column} ${like} ${quoteString(`%${filter.term}`)}`; + case 'regex_match': + return filter.caseSensitive + ? `regexp_matches(${column}, ${quoteString(filter.term)})` + : `regexp_matches(${column}, ${quoteString(filter.term)}, 'i')`; + } + break; + } + case 'set_membership': { + const values = filter.values.map((v) => renderValue(v, filter.columnType)).join(', '); + return `${column} ${filter.inclusive ? 'IN' : 'NOT IN'} (${values})`; + } + case 'is_null': + return `${column} IS NULL`; + case 'not_null': + return `${column} IS NOT NULL`; + case 'is_empty': + return `${column} = ''`; + case 'not_empty': + return `${column} <> ''`; + case 'is_true': + return `${column}`; + case 'is_false': + return `NOT ${column}`; + } + return undefined; +} + +/** Build the FROM clause, honouring import options that need explicit reader calls. */ +function renderFrom(filePath: string, extension: string, options: positron.DataImportOptions): string { + if ( + options.hasHeaderRow === false && + (extension === 'csv' || extension === 'tsv') + ) { + return `read_csv(${quoteString(filePath)}, header = false)`; + } + return quoteString(filePath); +} + +/** The ggsql data importer offered by the Data Explorer import dialog. */ +export const ggsqlDataImporter: positron.DataImporter = { + languageId: 'ggsql', + displayName: 'ggsql', + fileExtensions: READABLE_EXTENSIONS, + reservedNames: RESERVED_NAMES, + + generateCode(request: positron.DataImportRequest): positron.DataImportResult { + const filePath = request.fileUri.fsPath; + const extension = filePath.split('.').pop()?.toLowerCase() ?? ''; + const unsupported: string[] = []; + + if (request.options.sheetName !== undefined) { + unsupported.push(`Worksheet selection ('${request.options.sheetName}')`); + } + if ( + request.options.hasHeaderRow === false && + extension !== 'csv' && + extension !== 'tsv' + ) { + unsupported.push('Header row option (only supported for csv/tsv files)'); + } + + const lines = [ + `CREATE TABLE ${quoteIdentifier(request.variableName)} AS`, + 'SELECT *', + `FROM ${renderFrom(filePath, extension, request.options)}`, + ]; + + const view = request.view; + if (view && view.rowFilters.length > 0) { + const predicates: string[] = []; + for (const filter of view.rowFilters) { + const predicate = renderFilter(filter); + if (predicate === undefined) { + unsupported.push(`Row filter on ${filter.columnName} (${filter.filterType})`); + continue; + } + predicates.push(predicates.length === 0 ? predicate : `${filter.condition.toUpperCase()} ${predicate}`); + } + if (predicates.length > 0) { + lines.push(`WHERE ${predicates.join('\n ')}`); + } + } + + if (view && view.sortKeys.length > 0) { + const keys = view.sortKeys.map( + (key) => `${quoteIdentifier(key.columnName)} ${key.ascending ? 'ASC' : 'DESC'}` + ); + lines.push(`ORDER BY ${keys.join(', ')}`); + } + + return { + code: lines.join('\n') + ';', + unsupported: unsupported.length > 0 ? unsupported : undefined, + }; + }, +}; diff --git a/ggsql-vscode/src/extension.ts b/ggsql-vscode/src/extension.ts index b9d1c6be3..ba1d784cc 100644 --- a/ggsql-vscode/src/extension.ts +++ b/ggsql-vscode/src/extension.ts @@ -15,6 +15,8 @@ import { activateContextKeys } from './context'; import { parseCells } from './cellParser'; import { CELL_LANGUAGE_IDS, isGgsqlDocument } from './languages'; import { activateSqlAssociationPrompt } from './sqlAssociation'; +import { ggsqlDataImporter } from './dataImporter'; +import * as path from 'path'; // Output channel for logging const outputChannel = vscode.window.createOutputChannel('ggsql'); @@ -75,6 +77,28 @@ export function activate(context: vscode.ExtensionContext): void { log(`Registered ${drivers.length} connection drivers`); + // Register the ggsql data importer for the Data Explorer import dialog. + // Requires positron API >= 0.2.9; skip on older Positron builds. + if (typeof positronApi.dataExplorer?.registerDataImporter === 'function') { + context.subscriptions.push( + positronApi.dataExplorer.registerDataImporter(ggsqlDataImporter) + ); + log('Registered ggsql data importer'); + } else { + log('positron.dataExplorer.registerDataImporter not available - skipping data importer'); + } + + // Register the bundled agent skill root so agents discover the ggsql skill. + // Requires positron API >= 0.2.9; skip on older Positron builds. + if (typeof positronApi.ai?.registerAgentSkillRoot === 'function') { + context.subscriptions.push( + positronApi.ai.registerAgentSkillRoot(path.join(context.extensionPath, 'skills')) + ); + log('Registered ggsql agent skill root'); + } else { + log('positron.ai.registerAgentSkillRoot not available - skipping agent skill root'); + } + // Register "Source Current File" command for the editor run button context.subscriptions.push( vscode.commands.registerCommand('ggsql.sourceCurrentFile', async () => { diff --git a/ggsql-vscode/src/test/dataImporter.test.ts b/ggsql-vscode/src/test/dataImporter.test.ts new file mode 100644 index 000000000..3afc78012 --- /dev/null +++ b/ggsql-vscode/src/test/dataImporter.test.ts @@ -0,0 +1,155 @@ +import * as assert from 'assert'; +import { ggsqlDataImporter } from '../dataImporter'; +import type * as positron from '@posit-dev/positron'; +import type * as vscode from 'vscode'; + +function request( + filePath: string, + overrides: Partial = {}, +): positron.DataImportRequest { + return { + fileUri: { fsPath: filePath } as vscode.Uri, + variableName: 'my_table', + options: {}, + ...overrides, + }; +} + +/** Generate code synchronously, failing the test if the importer defers or declines. */ +function gen(req: positron.DataImportRequest): positron.DataImportResult { + const result = ggsqlDataImporter.generateCode(req); + assert.ok(result !== undefined && result !== null && !(result instanceof Promise)); + return result as positron.DataImportResult; +} + +suite('ggsqlDataImporter.generateCode', () => { + test('generates a plain import for a csv file', () => { + const result = gen(request('/data/penguins.csv')); + assert.strictEqual( + result?.code, + 'CREATE TABLE "my_table" AS\nSELECT *\nFROM \'/data/penguins.csv\';', + ); + assert.strictEqual(result?.unsupported, undefined); + }); + + test('escapes single quotes in file paths', () => { + const result = gen(request("/data/bob's file.csv")); + assert.ok(result?.code.includes("'/data/bob''s file.csv'")); + }); + + test('uses read_csv with header = false when hasHeaderRow is false', () => { + const result = gen( + request('/data/p.csv', { options: { hasHeaderRow: false } }), + ); + assert.ok(result?.code.includes("read_csv('/data/p.csv', header = false)")); + assert.strictEqual(result?.unsupported, undefined); + }); + + test('flags header option as unsupported for non-csv files', () => { + const result = gen( + request('/data/p.parquet', { options: { hasHeaderRow: false } }), + ); + assert.ok(result?.unsupported?.some((u) => u.includes('Header row'))); + }); + + test('flags worksheet selection as unsupported', () => { + const result = gen( + request('/data/p.csv', { options: { sheetName: 'Sheet2' } }), + ); + assert.ok(result?.unsupported?.some((u) => u.includes('Sheet2'))); + }); + + test('translates filters and sorts from the view', () => { + const result = gen( + request('/data/p.csv', { + view: { + rowFilters: [ + { + filterType: 'compare', + columnName: 'bill_length_mm', + columnType: 'number', + condition: 'and', + op: '>', + value: '40', + }, + { + filterType: 'set_membership', + columnName: 'species', + columnType: 'string', + condition: 'and', + values: ['Adelie', 'Chinstrap'], + inclusive: true, + }, + { + filterType: 'search', + columnName: 'island', + columnType: 'string', + condition: 'or', + searchType: 'contains', + term: 'Tor', + caseSensitive: false, + }, + ], + sortKeys: [ + { columnName: 'bill_length_mm', ascending: false }, + ], + }, + }), + ); + assert.strictEqual( + result?.code, + 'CREATE TABLE "my_table" AS\n' + + 'SELECT *\n' + + "FROM '/data/p.csv'\n" + + 'WHERE "bill_length_mm" > 40\n' + + ' AND "species" IN (\'Adelie\', \'Chinstrap\')\n' + + ' OR "island" ILIKE \'%Tor%\'\n' + + 'ORDER BY "bill_length_mm" DESC;', + ); + assert.strictEqual(result?.unsupported, undefined); + }); + + test('translates remaining filter kinds', () => { + const result = gen( + request('/data/p.csv', { + view: { + rowFilters: [ + { filterType: 'between', columnName: 'x', columnType: 'integer', condition: 'and', leftValue: '1', rightValue: '5' }, + { filterType: 'not_between', columnName: 'y', columnType: 'integer', condition: 'and', leftValue: '0', rightValue: '10' }, + { filterType: 'is_null', columnName: 'z', columnType: 'string', condition: 'and' }, + { filterType: 'not_null', columnName: 'w', columnType: 'string', condition: 'and' }, + { filterType: 'is_empty', columnName: 'v', columnType: 'string', condition: 'and' }, + { filterType: 'is_true', columnName: 'b', columnType: 'boolean', condition: 'and' }, + ], + sortKeys: [], + }, + }), + ); + assert.strictEqual( + result?.code, + 'CREATE TABLE "my_table" AS\n' + + 'SELECT *\n' + + "FROM '/data/p.csv'\n" + + 'WHERE "x" BETWEEN 1 AND 5\n' + + ' AND "y" NOT BETWEEN 0 AND 10\n' + + ' AND "z" IS NULL\n' + + ' AND "w" IS NOT NULL\n' + + ' AND "v" = \'\'\n' + + ' AND "b";', + ); + }); + + test('renders regex search with case-insensitive flag when needed', () => { + const result = gen( + request('/data/p.csv', { + view: { + rowFilters: [ + { filterType: 'search', columnName: 'name', columnType: 'string', condition: 'and', searchType: 'regex_match', term: '^a+$', caseSensitive: false }, + ], + sortKeys: [], + }, + }), + ); + assert.ok(result?.code.includes(`regexp_matches("name", '^a+$', 'i')`)); + }); +}); From a3b10de756956af29f97b170224f29d00bed84a6 Mon Sep 17 00:00:00 2001 From: Thomas Lin Pedersen Date: Tue, 29 Sep 2026 08:01:46 +0200 Subject: [PATCH 2/2] Move facet scale training to core --- CHANGELOG.md | 7 + src/execute/mod.rs | 188 +++++++++++++++-- src/execute/scale.rs | 279 ++++++++++++++++++++++++- src/parser/builder.rs | 1 + src/plot/facet/mod.rs | 5 + src/plot/facet/panels.rs | 307 ++++++++++++++++++++++++++++ src/plot/scale/mod.rs | 2 +- src/plot/scale/types.rs | 31 ++- src/writer/hephaestus/CLAUDE.md | 10 +- src/writer/hephaestus/compose.rs | 10 +- src/writer/hephaestus/facet.rs | 237 ++++------------------ src/writer/hephaestus/scales.rs | 337 +++++++++++-------------------- src/writer/vegalite/encoding.rs | 11 + src/writer/vegalite/layer.rs | 3 + src/writer/vegalite/mod.rs | 98 +++++++++ 15 files changed, 1083 insertions(+), 443 deletions(-) create mode 100644 src/plot/facet/panels.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 53e92ca60..81594259c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,13 @@ ### Fixed +- Free facet dimensions now resolve their domain, breaks, labels, and minor + breaks per panel in core (`Scale::panels`, indexed by the canonical panel + order shared by both writers). The Vega-Lite writer no longer pins the + globally resolved break set as `axis.values` on a free dimension — which had + left most panels showing a single tick — and the hephaestus writer consumes + the core-resolved per-panel scales instead of deriving panel extents itself + (#516). - Fixed a parser bug that interpreted comment characters inside string literals as initializing a comment (#555). - Fixed a bug in stat_aggregate prevented transposed layers from properly diff --git a/src/execute/mod.rs b/src/execute/mod.rs index 9f633d10f..d0bc4fbc9 100644 --- a/src/execute/mod.rs +++ b/src/execute/mod.rs @@ -1561,19 +1561,10 @@ pub fn prepare_data_with_reader(query: &str, reader: &dyn Reader) -> Result Result 'x'"; + let result = prepare_data_with_reader(query, &reader).unwrap(); + let spec = &result.specs[0]; + + let scale = spec.find_scale("pos1").expect("pos1 scale"); + let panels = scale.panels.as_ref().expect("free x should resolve panels"); + assert_eq!(panels.len(), 2, "one entry per panel"); + + let domain = |p: &Option| { + let p = p.as_ref().expect("non-empty panel"); + ( + p.input_range.first().unwrap().to_f64().unwrap(), + p.input_range.last().unwrap().to_f64().unwrap(), + ) + }; + let (lo_a, hi_a) = domain(&panels[0]); + let (lo_b, hi_b) = domain(&panels[1]); + + // Each panel's domain covers only its own data (with expansion), and + // the two panels resolve to disjoint domains. + assert!( + lo_a <= 1.0 && hi_a >= 3.0 && hi_a < 50.0, + "panel A: {lo_a}..{hi_a}" + ); + assert!( + lo_b > 50.0 && lo_b <= 100.0 && hi_b >= 102.0, + "panel B: {lo_b}..{hi_b}" + ); + + // Breaks are resolved per panel and land inside the panel's domain. + for (p, (lo, hi)) in panels.iter().zip([(lo_a, hi_a), (lo_b, hi_b)]) { + let p = p.as_ref().unwrap(); + assert!(!p.breaks.is_empty(), "panel should have its own breaks"); + for (pos, _) in &p.breaks { + assert!( + *pos >= lo && *pos <= hi, + "break {pos} outside panel domain {lo}..{hi}" + ); + } + } + + // A fixed dimension stays shared: pos2 gets no per-panel resolution. + let y_scale = spec.find_scale("pos2").expect("pos2 scale"); + assert!(y_scale.panels.is_none()); + } + + #[cfg(feature = "duckdb")] + #[test] + fn test_free_facet_explicit_domain_applies_to_all_panels() { + // An explicit user FROM domain is not per-panel: it applies uniformly, + // even on a free dimension. + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + reader + .connection() + .execute( + "CREATE TABLE panel_explicit AS SELECT * FROM (VALUES + (1.0, 10.0, 'A'), + (100.0, 40.0, 'B') + ) AS t(x, y, g)", + duckdb::params![], + ) + .unwrap(); + + let query = "SELECT * FROM panel_explicit + VISUALISE x, y DRAW point FACET g SETTING free => 'x' + SCALE x FROM (0, 200)"; + let result = prepare_data_with_reader(query, &reader).unwrap(); + let scale = result.specs[0].find_scale("pos1").expect("pos1 scale"); + let panels = scale.panels.as_ref().expect("panels"); + assert_eq!(panels.len(), 2); + for p in panels { + let p = p.as_ref().unwrap(); + let lo = p.input_range.first().unwrap().to_f64().unwrap(); + let hi = p.input_range.last().unwrap().to_f64().unwrap(); + assert_eq!((lo, hi), (0.0, 200.0)); + } + } + + #[cfg(feature = "duckdb")] + #[test] + fn test_free_facet_discrete_domain_narrowed_per_panel() { + // A free discrete dimension narrows the global domain to the + // categories present in each panel, in the global order. + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + reader + .connection() + .execute( + "CREATE TABLE panel_discrete AS SELECT * FROM (VALUES + ('b', 10.0, 'A'), + ('a', 20.0, 'A'), + ('c', 40.0, 'B') + ) AS t(x, y, g)", + duckdb::params![], + ) + .unwrap(); + + let query = + "SELECT * FROM panel_discrete VISUALISE x, y DRAW point FACET g SETTING free => 'x'"; + let result = prepare_data_with_reader(query, &reader).unwrap(); + let scale = result.specs[0].find_scale("pos1").expect("pos1 scale"); + let panels = scale.panels.as_ref().expect("panels"); + assert_eq!(panels.len(), 2); + + let categories = |p: &Option| { + p.as_ref() + .unwrap() + .input_range + .iter() + .map(|e| e.to_key_string()) + .collect::>() + }; + // Global order is alphabetical; panel A holds a and b, panel B only c. + assert_eq!(categories(&panels[0]), vec!["a", "b"]); + assert_eq!(categories(&panels[1]), vec!["c"]); + } + + #[cfg(feature = "duckdb")] + #[test] + fn test_fixed_facet_resolves_no_panels() { + let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap(); + reader + .connection() + .execute( + "CREATE TABLE panel_fixed AS SELECT * FROM (VALUES + (1.0, 10.0, 'A'), + (100.0, 40.0, 'B') + ) AS t(x, y, g)", + duckdb::params![], + ) + .unwrap(); + + let query = "SELECT * FROM panel_fixed VISUALISE x, y DRAW point FACET g"; + let result = prepare_data_with_reader(query, &reader).unwrap(); + let scale = result.specs[0].find_scale("pos1").expect("pos1 scale"); + assert!(scale.panels.is_none(), "fixed facet keeps a shared scale"); + } + #[cfg(feature = "duckdb")] #[test] fn test_prepare_data_no_viz() { diff --git a/src/execute/scale.rs b/src/execute/scale.rs index 825ab7c51..306009140 100644 --- a/src/execute/scale.rs +++ b/src/execute/scale.rs @@ -983,6 +983,17 @@ pub fn resolve_scales(spec: &mut Plot, data_map: &mut HashMap }) .unwrap_or((false, false)); + let free_aesthetics: HashSet = match &spec.facet { + Some(facet) => spec + .scales + .iter() + .filter(|s| facet.is_free(&s.aesthetic)) + .map(|s| s.aesthetic.clone()) + .collect(), + None => HashSet::new(), + }; + let mut panel_templates: HashMap = HashMap::new(); + for idx in 0..spec.scales.len() { // Clone aesthetic to avoid borrow issues with find_columns_for_aesthetic let aesthetic = spec.scales[idx].aesthetic.clone(); @@ -993,6 +1004,10 @@ pub fn resolve_scales(spec: &mut Plot, data_map: &mut HashMap continue; } + if free_aesthetics.contains(&aesthetic) { + panel_templates.insert(aesthetic.clone(), spec.scales[idx].clone()); + } + // Infer target type and coerce columns if needed // This enables e.g. SCALE DISCRETE color FROM [true, false] to coerce string "true"/"false" to boolean if let Some(target_type) = infer_scale_target_type(&spec.scales[idx]) { @@ -1046,9 +1061,247 @@ pub fn resolve_scales(spec: &mut Plot, data_map: &mut HashMap } } + // Per-panel resolution for positional scales on a free facet dimension. + if !free_aesthetics.is_empty() { + resolve_panel_scales( + spec, + data_map, + &free_aesthetics, + panel_templates, + is_polar && polar_is_full_circle, + )?; + } + + Ok(()) +} + +/// Resolve per-panel domains, breaks, and labels for every positional scale on +/// a `free` facet dimension, storing them in [`Scale::panels`] (indexed by the +/// canonical panel order, see [`crate::plot::facet::panels`]). +/// +/// Writers consume these verbatim: they must not derive per-panel positional +/// extents themselves. Each panel is resolved with the *same* machinery as the +/// global scale (`ScaleType::resolve`) run over the panel's own rows, so a free +/// panel gets exactly the domain, expansion, breaks, and labels a fixed axis +/// over the same data would get. Two exceptions: +/// +/// - **Discrete/ordinal** domains are the global domain narrowed to the +/// categories present in the panel, kept in the global order (rather than the +/// panel's first-seen order), so panels agree on level placement. +/// - **Binned** scales keep the globally resolved bin edges — bins are +/// resolved pre-stat and must not be recomputed per panel — narrowed to the +/// window of bins the panel's data occupies. +/// +/// A scale with an explicit user `FROM` domain resolves to that same domain in +/// every panel (explicit settings apply uniformly; only computed values are +/// per-panel). An empty panel gets a `None` entry; consumers fall back to the +/// shared scale there. +fn resolve_panel_scales( + spec: &mut Plot, + data_map: &HashMap, + free_aesthetics: &HashSet, + templates: HashMap, + polar_zero_expand: bool, +) -> Result<()> { + use crate::plot::facet::panels; + use crate::plot::scale::ScaleDataContext; + + let layer0 = data_map.get(&naming::layer_key(0)).ok_or_else(|| { + GgsqlError::InternalError("Missing layer 0 data for panel scale resolution".to_string()) + })?; + let panel_keys = panels::panel_keys(spec, layer0)?; + if panel_keys.is_empty() { + return Ok(()); + } + let aesthetic_ctx = spec.get_aesthetic_context(); + + for aesthetic in free_aesthetics { + let Some(idx) = spec.scales.iter().position(|s| &s.aesthetic == aesthetic) else { + continue; + }; + let Some(st) = spec.scales[idx].scale_type.clone() else { + continue; + }; + let kind = st.scale_type_kind(); + // An identity scale passes values through; there is no domain to free. + if kind == ScaleTypeKind::Identity { + continue; + } + + let locations = + find_column_locations_for_aesthetic(&spec.layers, aesthetic, data_map, &aesthetic_ctx); + if locations.is_empty() { + continue; + } + + let mut entries: Vec> = + Vec::with_capacity(panel_keys.len()); + for pk in &panel_keys { + // Slice every training column to this panel's rows. A layer with no + // facet column belongs to every panel whole. + let mut owned: Vec = Vec::with_capacity(locations.len()); + let mut total_rows = 0usize; + for (layer_key, column) in &locations { + let df = data_map.get(layer_key).ok_or_else(|| { + GgsqlError::InternalError(format!( + "Missing data for layer '{}' during panel scale resolution", + layer_key + )) + })?; + let array = df.column(column).map_err(|e| { + GgsqlError::InternalError(format!( + "Missing column '{}' during panel scale resolution: {}", + column, e + )) + })?; + match panels::rows_in_panel(df, pk)? { + Some(row_idx) => { + total_rows += row_idx.len(); + owned.push(panels::take_rows(array, &row_idx)?); + } + None => { + total_rows += array.len(); + owned.push(array.clone()); + } + } + } + if total_rows == 0 { + entries.push(None); + continue; + } + + let entry = if kind == ScaleTypeKind::Binned { + resolve_binned_panel(&spec.scales[idx], &owned) + } else { + let column_refs: Vec<&ArrayRef> = owned.iter().collect(); + let mut context = + ScaleDataContext::from_columns(&column_refs, st.uses_discrete_input_range()); + if polar_zero_expand && aesthetic == "pos2" { + context.default_expand = Some((0.0, 0.0)); + } + let mut panel_scale = templates.get(aesthetic).cloned().ok_or_else(|| { + GgsqlError::InternalError(format!( + "Missing pre-resolution template for free scale '{}'", + aesthetic + )) + })?; + let display_aes = aesthetic_ctx.map_internal_to_user(aesthetic); + st.resolve(&mut panel_scale, &context, aesthetic) + .map_err(|e| { + GgsqlError::ValidationError(format!("Scale '{}': {}", display_aes, e)) + })?; + + // Keep the panel's categories in the *global* domain order, so + // every panel places a given level at the same position. + if matches!(kind, ScaleTypeKind::Discrete | ScaleTypeKind::Ordinal) { + narrow_discrete_domain(&mut panel_scale, &spec.scales[idx]); + } + + Some(crate::plot::PanelScale { + input_range: panel_scale.input_range.clone().unwrap_or_default(), + breaks: panel_scale.labelled_breaks(), + minor_breaks: panel_scale.numeric_minor_breaks(), + }) + }; + entries.push(entry); + } + spec.scales[idx].panels = Some(entries); + } + Ok(()) } +/// Narrow a per-panel resolved discrete/ordinal domain to the categories that +/// also exist in the global domain, in the global domain's order. +fn narrow_discrete_domain(panel_scale: &mut Scale, global_scale: &Scale) { + let (Some(global_range), Some(panel_range)) = ( + global_scale.input_range.as_ref(), + panel_scale.input_range.as_ref(), + ) else { + return; + }; + let present: HashSet = panel_range.iter().map(|e| e.to_key_string()).collect(); + panel_scale.input_range = Some( + global_range + .iter() + .filter(|e| present.contains(&e.to_key_string())) + .cloned() + .collect(), + ); +} + +/// Per-panel resolution for a **binned** free dimension: the globally resolved +/// bin edges, narrowed to the window of bins the panel's data occupies. Core +/// never invents bin boundaries — it only selects from the edges the global +/// scale resolved. Edges and domain narrow together because a binned axis +/// derives band width from its edge count. +/// +/// `None` when the scale has no usable bins or the panel has no finite extent. +fn resolve_binned_panel(scale: &Scale, columns: &[ArrayRef]) -> Option { + use crate::array_util::cast_array; + + let Some(ParameterValue::Array(edge_elements)) = scale.properties.get("breaks") else { + return None; + }; + if edge_elements.len() < 2 { + return None; + } + let edges: Vec = edge_elements.iter().filter_map(|e| e.to_f64()).collect(); + if edges.len() != edge_elements.len() { + return None; + } + + // The panel's finite numeric extent across all training columns. + let (mut lo, mut hi) = (f64::INFINITY, f64::NEG_INFINITY); + for array in columns { + let casted = cast_array(array, &arrow::datatypes::DataType::Float64).ok()?; + let values = crate::array_util::as_f64(&casted).ok()?; + for v in values.iter().flatten() { + if v.is_finite() { + lo = lo.min(v); + hi = hi.max(v); + } + } + } + if !(lo.is_finite() && hi.is_finite()) { + return None; + } + + // The inclusive window of bins covering the panel's extent: the bin whose + // lower edge starts at or below `lo` through the bin whose upper edge + // reaches `hi`. + let n = edges.len(); + let first = (0..n - 1).rev().find(|&i| edges[i] <= lo).unwrap_or(0); + let last = (1..n) + .find(|&j| edges[j] >= hi) + .unwrap_or(n - 1) + .max(first + 1) + .min(n - 1); + + let window_elements: Vec = edge_elements[first..=last].to_vec(); + let (lo_edge, hi_edge) = (edges[first], edges[last]); + let breaks = scale + .labelled_breaks() + .into_iter() + .filter(|(pos, _)| *pos >= lo_edge && *pos <= hi_edge) + .collect(); + + Some(crate::plot::PanelScale { + input_range: vec![ + window_elements + .first() + .cloned() + .unwrap_or(ArrayElement::Null), + window_elements + .last() + .cloned() + .unwrap_or(ArrayElement::Null), + ], + breaks, + minor_breaks: None, + }) +} + /// Find all columns for an aesthetic (including family members like xmin/xmax for "x"). /// Each mapping is looked up in its corresponding data source. /// Returns references to the Columns found. @@ -1060,7 +1313,22 @@ pub fn find_columns_for_aesthetic<'a>( data_map: &'a HashMap, aesthetic_ctx: &AestheticContext, ) -> Vec<&'a ArrayRef> { - let mut column_refs = Vec::new(); + find_column_locations_for_aesthetic(layers, aesthetic, data_map, aesthetic_ctx) + .into_iter() + .filter_map(|(layer_key, name)| data_map.get(&layer_key)?.column(&name).ok()) + .collect() +} + +/// Like [`find_columns_for_aesthetic`], but returns `(layer_key, column_name)` +/// locations instead of column references, so callers can slice the columns +/// (e.g., to a facet panel's rows) before reading them. +fn find_column_locations_for_aesthetic( + layers: &[Layer], + aesthetic: &str, + data_map: &HashMap, + aesthetic_ctx: &AestheticContext, +) -> Vec<(String, String)> { + let mut locations = Vec::new(); let aesthetics_to_check = aesthetic_ctx .internal_position_family(aesthetic) .map(|f| f.to_vec()) @@ -1068,12 +1336,13 @@ pub fn find_columns_for_aesthetic<'a>( // Check each layer's mapping - every layer has its own data for (i, layer) in layers.iter().enumerate() { - if let Some(df) = data_map.get(&naming::layer_key(i)) { + let layer_key = naming::layer_key(i); + if let Some(df) = data_map.get(&layer_key) { for aes_name in &aesthetics_to_check { if let Some(AestheticValue::Column { name, .. }) = layer.mappings.get(aes_name) { // Regular columns (data and position annotations) participate in scale training - if let Ok(column) = df.column(name) { - column_refs.push(column); + if df.column(name).is_ok() { + locations.push((layer_key.clone(), name.clone())); } } // AnnotationColumn and Literal don't participate in scale training @@ -1081,7 +1350,7 @@ pub fn find_columns_for_aesthetic<'a>( } } - column_refs + locations } // ============================================================================= diff --git a/src/parser/builder.rs b/src/parser/builder.rs index d2f44e735..9952e25bd 100644 --- a/src/parser/builder.rs +++ b/src/parser/builder.rs @@ -759,6 +759,7 @@ fn build_scale(node: &Node, source: &SourceTree) -> Result { resolved: false, label_mapping, label_template, + panels: None, }) } diff --git a/src/plot/facet/mod.rs b/src/plot/facet/mod.rs index 4946f8346..9ff130127 100644 --- a/src/plot/facet/mod.rs +++ b/src/plot/facet/mod.rs @@ -2,8 +2,13 @@ //! //! This module defines faceting configuration for small multiples. +pub mod panels; mod resolve; mod types; +pub use panels::{ + element_to_key, is_binned, level_keys, ordered_levels, panel_keys, rows_in_panel, FacetLevel, + LevelKey, PanelKey, +}; pub use resolve::{resolve_properties, FacetDataContext}; pub use types::{Facet, FacetLayout}; diff --git a/src/plot/facet/panels.rs b/src/plot/facet/panels.rs new file mode 100644 index 000000000..010340483 --- /dev/null +++ b/src/plot/facet/panels.rs @@ -0,0 +1,307 @@ +//! Canonical facet panel assignment and ordering. +//! +//! This is the single source of truth for which rows belong to which panel and +//! in what order panels are enumerated. Writers must use these functions rather +//! than deriving their own panel sets, so that per-panel structures resolved in +//! core — [`Scale::panels`](crate::plot::Scale::panels) — line up with the +//! panels a writer draws. +//! +//! Panel enumeration order (the canonical contract): +//! - Wrap: the ordered levels of `facet1`, flowed row-major. +//! - Grid: row-major over (`facet1` row, `facet2` column) — `facet1` is the +//! outer loop. +//! +//! Level ordering follows the facet aesthetic's `SCALE` (its `input_range`, +//! then `reverse`), falling back to a numeric-aware ascending sort of the +//! values present. A binned facet sorts by its (numeric) bin centre, with NULL +//! (censored) panels last. + +use std::cmp::Ordering; +use std::collections::HashSet; + +use arrow::array::{Array, ArrayRef, StringArray, UInt32Array}; +use arrow::datatypes::DataType; + +use crate::array_util::{as_f64, as_str, cast_array}; +use crate::plot::{ArrayElement, ParameterValue, Scale, ScaleTypeKind}; +use crate::{DataFrame, GgsqlError, Plot, Result}; + +use super::types::FacetLayout; + +/// The value selecting one facet cell's rows: the facet column's text form, +/// paired with whether the cell was NULL. +/// +/// Text alone would make a genuine empty-string category and a NULL the same +/// panel, so the null flag travels alongside the text everywhere a level is +/// identified or matched. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct LevelKey { + pub text: String, + pub is_null: bool, +} + +/// One distinct facet level: the key selecting its rows and the numeric form of +/// the same cell (the bin-centre join key for a binned facet). +#[derive(Debug, Clone)] +pub struct FacetLevel { + pub key: LevelKey, + pub value: f64, +} + +/// One panel in the canonical enumeration: which facet values select its rows. +/// `facet2` is `None` for Wrap layouts. +#[derive(Debug, Clone)] +pub struct PanelKey { + /// 0-based panel index in the canonical order. + pub index: usize, + pub facet1: LevelKey, + pub facet2: Option, +} + +/// Read a column as strings, casting non-text columns to text. Nulls become +/// empty strings; pair with the array's own null bitmap when the distinction +/// matters (see [`level_keys`]). +fn column_to_strings(df: &DataFrame, name: &str) -> Result> { + let array = df.column(name)?; + let casted; + let str_array: &StringArray = if matches!(array.data_type(), DataType::Utf8) { + as_str(array)? + } else { + casted = cast_array(array, &DataType::Utf8)?; + as_str(&casted)? + }; + Ok((0..str_array.len()) + .map(|i| { + if str_array.is_null(i) { + String::new() + } else { + str_array.value(i).to_string() + } + }) + .collect()) +} + +/// Read a column as `f64`, casting from any numeric/temporal source type and +/// mapping nulls to `NaN`. +fn column_to_f64(df: &DataFrame, name: &str) -> Result> { + let array = df.column(name)?; + let casted; + let f64_array = if matches!(array.data_type(), DataType::Float64) { + as_f64(array)? + } else { + casted = cast_array(array, &DataType::Float64)?; + as_f64(&casted)? + }; + Ok(f64_array.iter().map(|v| v.unwrap_or(f64::NAN)).collect()) +} + +/// The per-row facet key of one facet column: its text form paired with the +/// null bitmap, so a NULL cell selects a different panel than an empty-string +/// category — see [`LevelKey`]. +pub fn level_keys(df: &DataFrame, column: &str) -> Result> { + let array = df.column(column)?; + Ok(column_to_strings(df, column)? + .into_iter() + .enumerate() + .map(|(i, text)| LevelKey { + text, + is_null: array.is_null(i), + }) + .collect()) +} + +/// Distinct facet levels present in the data, ordered per the facet scale. +/// +/// * `internal_aes` is the internal facet aesthetic name (`"facet1"` / +/// `"facet2"`); the column read is its aesthetic-renamed form. +pub fn ordered_levels(spec: &Plot, df: &DataFrame, internal_aes: &str) -> Result> { + let col = crate::naming::aesthetic_column(internal_aes); + let keys = level_keys(df, &col)?; + // The numeric form of the same column, for the binned join. A text facet + // column can't cast; `value` is only read for binned scales. + let values = column_to_f64(df, &col).unwrap_or_else(|_| vec![f64::NAN; keys.len()]); + + let mut seen = HashSet::new(); + let mut distinct: Vec = Vec::new(); + for (i, key) in keys.iter().enumerate() { + if seen.insert(key.clone()) { + distinct.push(FacetLevel { + key: key.clone(), + value: values[i], + }); + } + } + + let scale = spec.find_scale(internal_aes); + Ok(order_levels(distinct, scale)) +} + +/// Order distinct facet levels: a binned facet sorts by its (numeric) bin +/// centre; everything else follows the scale's `input_range`, then any +/// present-but-unlisted values sorted numeric-aware ascending. Reversed when +/// the scale sets `reverse => true`. +fn order_levels(mut distinct: Vec, scale: Option<&Scale>) -> Vec { + let reverse = matches!( + scale.and_then(|s| s.properties.get("reverse")), + Some(ParameterValue::Boolean(true)) + ); + + let mut ordered = if is_binned(scale) { + // Bin centres sort naturally; NULL (censored) panels go last. + distinct.sort_by(|a, b| match (a.value.is_finite(), b.value.is_finite()) { + (true, true) => a.value.total_cmp(&b.value), + (true, false) => Ordering::Less, + (false, true) => Ordering::Greater, + (false, false) => Ordering::Equal, + }); + distinct + } else { + match scale.and_then(|s| s.input_range.as_ref()) { + Some(range) => { + let order: Vec = range.iter().map(element_to_key).collect(); + let (mut ranked, mut extra): (Vec, Vec) = + (Vec::new(), Vec::new()); + for key in &order { + if let Some(pos) = distinct.iter().position(|l| &l.key == key) { + ranked.push(distinct.remove(pos)); + } + } + extra.extend(distinct); + sort_levels(&mut extra); + ranked.extend(extra); + ranked + } + None => { + sort_levels(&mut distinct); + distinct + } + } + }; + if reverse { + ordered.reverse(); + } + ordered +} + +/// Numeric-aware ascending sort by key: numeric when every key parses as `f64`, +/// otherwise lexical. +fn sort_levels(levels: &mut [FacetLevel]) { + if levels.iter().all(|l| l.key.text.parse::().is_ok()) { + levels.sort_by(|a, b| { + let a = a.key.text.parse::().unwrap(); + let b = b.key.text.parse::().unwrap(); + a.partial_cmp(&b).unwrap_or(Ordering::Equal) + }); + } else { + levels.sort_by(|a, b| a.key.text.cmp(&b.key.text)); + } +} + +/// Whether a facet scale is binned (numeric/temporal facet columns default to +/// it). +pub fn is_binned(scale: Option<&Scale>) -> bool { + scale + .and_then(|s| s.scale_type.as_ref()) + .map(|st| st.scale_type_kind()) + == Some(ScaleTypeKind::Binned) +} + +/// An `input_range` element as the key the facet column carries for it: the +/// text form the column casts to (whole numbers as integers, matching an +/// integer column's cast to text), plus whether the element is the null level. +pub fn element_to_key(element: &ArrayElement) -> LevelKey { + let text = match element { + ArrayElement::String(s) => s.clone(), + ArrayElement::Number(n) if n.fract() == 0.0 && n.is_finite() => format!("{}", *n as i64), + ArrayElement::Number(n) => n.to_string(), + ArrayElement::Boolean(b) => b.to_string(), + ArrayElement::Null => String::new(), + other => format!("{other:?}"), + }; + LevelKey { + text, + is_null: matches!(element, ArrayElement::Null), + } +} + +/// The panels of a faceted plot, in the canonical enumeration order. +/// +/// `layer0` is the first layer's data, which defines the panel set (a layer +/// contributing a level no other layer has still gets its panel via layer 0's +/// facet columns when it participates in the plot's facet; layers missing the +/// facet column entirely are repeated in every panel — see [`rows_in_panel`]). +/// +/// Returns an empty vector when the plot has no `FACET` or the data has no +/// facet levels; writers collapse that case to a single panel bound to the +/// shared scales. +pub fn panel_keys(spec: &Plot, layer0: &DataFrame) -> Result> { + let Some(facet) = &spec.facet else { + return Ok(Vec::new()); + }; + match &facet.layout { + FacetLayout::Wrap { .. } => { + let levels = ordered_levels(spec, layer0, "facet1")?; + Ok(levels + .into_iter() + .enumerate() + .map(|(index, level)| PanelKey { + index, + facet1: level.key, + facet2: None, + }) + .collect()) + } + FacetLayout::Grid { .. } => { + let rows = ordered_levels(spec, layer0, "facet1")?; + let cols = ordered_levels(spec, layer0, "facet2")?; + let mut panels = Vec::with_capacity(rows.len() * cols.len()); + let mut index = 0; + for row in &rows { + for col in &cols { + panels.push(PanelKey { + index, + facet1: row.key.clone(), + facet2: Some(col.key.clone()), + }); + index += 1; + } + } + Ok(panels) + } + } +} + +/// The row indices of `df` belonging to `panel`, or `None` when `df` has no +/// facet1 column — an annotation/global layer, which belongs to every panel +/// whole. +pub fn rows_in_panel(df: &DataFrame, panel: &PanelKey) -> Result>> { + let f1 = crate::naming::aesthetic_column("facet1"); + if df.column(&f1).is_err() { + return Ok(None); + } + let c1 = level_keys(df, &f1)?; + let c2 = match &panel.facet2 { + Some(_) => Some(level_keys(df, &crate::naming::aesthetic_column("facet2"))?), + None => None, + }; + + let mut idx: Vec = Vec::new(); + for i in 0..df.height() { + if c1[i] != panel.facet1 { + continue; + } + if let (Some(c2), Some(want2)) = (&c2, &panel.facet2) { + if &c2[i] != want2 { + continue; + } + } + idx.push(i as u32); + } + Ok(Some(idx)) +} + +/// Take `indices` rows of an array, preserving its type. +pub fn take_rows(array: &ArrayRef, indices: &[u32]) -> Result { + arrow::compute::take(array.as_ref(), &UInt32Array::from(indices.to_vec()), None) + .map_err(|e| GgsqlError::InternalError(format!("Failed to take panel rows: {}", e))) +} diff --git a/src/plot/scale/mod.rs b/src/plot/scale/mod.rs index fe12bc7e4..9065818df 100644 --- a/src/plot/scale/mod.rs +++ b/src/plot/scale/mod.rs @@ -27,7 +27,7 @@ pub use scale_type::{ pub use shape::shape_to_svg_path; pub use transform::{Transform, TransformKind, TransformTrait, ALL_TRANSFORM_NAMES}; -pub use types::{OutputRange, Scale}; +pub use types::{OutputRange, PanelScale, Scale}; use crate::plot::{ArrayElement, ArrayElementType}; diff --git a/src/plot/scale/types.rs b/src/plot/scale/types.rs index 3d9786ea4..51350503e 100644 --- a/src/plot/scale/types.rs +++ b/src/plot/scale/types.rs @@ -80,6 +80,34 @@ pub struct Scale { /// Example: "{} units" -> {"0": "0 units", "25": "25 units", ...} #[serde(default = "default_label_template")] pub label_template: String, + /// Per-panel resolution for a positional scale whose facet dimension is + /// `free`, indexed by panel index in the canonical order (see + /// [`facet::panels`](crate::plot::facet::panels)). `None` when the scale is + /// shared across panels (no facet, or the dimension is fixed). A `None` + /// entry is an empty panel; consumers fall back to the shared scale there. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub panels: Option>>, +} + +/// The resolution of one positional scale within one facet panel: domain, +/// breaks, and labels, all computed from the panel's own rows by core. +/// +/// Writers read this verbatim for free facet dimensions instead of deriving +/// per-panel extents themselves. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct PanelScale { + /// Per-panel domain: `[min, max]` for continuous/binned (expanded for + /// continuous, the narrowed bin-edge window for binned), or the narrowed + /// category list for discrete/ordinal, in the global domain's order. + pub input_range: Vec, + /// Per-panel major breaks as `(position, label)` pairs. A `None` label was + /// explicitly suppressed (`RENAMING ... => NULL`): keep the break on + /// categorical scales (blank text), drop it on numeric ones. + pub breaks: Vec<(f64, Option)>, + /// Per-panel minor break positions. Mirrors + /// [`Scale::numeric_minor_breaks`]: `Some(vec![])` means "resolved to no + /// minors" and must be honoured; `None` means none were resolved. + pub minor_breaks: Option>, } impl Scale { @@ -97,6 +125,7 @@ impl Scale { resolved: false, label_mapping: None, label_template: "{}".to_string(), + panels: None, } } @@ -181,7 +210,7 @@ impl Scale { /// Breaks paired with their resolved label, where `None` means the label was /// explicitly suppressed (as opposed to merely empty). The two public break /// accessors differ only in what they do with that `None`. - fn labelled_breaks(&self) -> Vec<(f64, Option)> { + pub(crate) fn labelled_breaks(&self) -> Vec<(f64, Option)> { let raw = match &self.scale_type { Some(st) => st.break_labels(self), None => self diff --git a/src/writer/hephaestus/CLAUDE.md b/src/writer/hephaestus/CLAUDE.md index 3e7e716e7..875d0faec 100644 --- a/src/writer/hephaestus/CLAUDE.md +++ b/src/writer/hephaestus/CLAUDE.md @@ -78,14 +78,18 @@ missing *pass-through*, not a better computation here. The same shape held upstream — every hephaestus gap this writer hit was a missing setter, not a missing algorithm. -There are exactly **two scoped exceptions**, both flagged in the code and both -debt that would disappear if ggsql resolved more: +There is exactly **one scoped exception**, flagged in the code: | Exception | Where | Why | | --- | --- | --- | -| Free facet dimensions | `scales::{free_position_scale, free_binned_scale}` | ggsql resolves one global domain; a `free` panel needs its own. Only the *extent* is computed — the padding around it is still ggsql's, via `Scale::expand_range`. | | Spatial `pos1`/`pos2` | `compose.rs::map_bbox` | A map's frame is `Projection.computed["bbox"]`, in the target CRS, with `SCALE lon`/`lat` limits already folded in. A resolved `pos1`/`pos2` is *not* the alternative: for a map, ggsql resolves those against the graticule extent in EPSG:4326, so their domain is degrees and their breaks are graticule positions, not the frame. Only a bare `spatial` geom with no `PROJECT` falls back to the geometry extent. | +Free facet dimensions are **not** an exception: ggsql resolves the per-panel +domain, breaks, labels, and minor breaks in core (`Scale::panels`, indexed by +the canonical panel order in `plot::facet::panels`), and +`scales::free_position_scale` only translates that entry into a hephaestus +scale. The writer never derives a positional extent. + ## Configuration Rendering needs concrete dimensions, so unlike the Vega-Lite writer these diff --git a/src/writer/hephaestus/compose.rs b/src/writer/hephaestus/compose.rs index 137e50a02..eb266659e 100644 --- a/src/writer/hephaestus/compose.rs +++ b/src/writer/hephaestus/compose.rs @@ -171,13 +171,11 @@ pub fn build_composition( .collect::>()?; let empty = slices.iter().all(|(_, df)| df.height() == 0); - // Fixed dimensions bind the shared `pos1`/`pos2`; free ones get a - // per-panel scale over this panel's slices — the one place the writer - // computes an extent of its own. + // Fixed dimensions bind the shared `pos1`/`pos2`; free ones get the + // per-panel scale core resolved for this panel (`Scale::panels`). let mut ps = facet::PanelScales::new(spec, panel); - let layer_dfs: Vec<&DataFrame> = slices.iter().map(|(_, df)| df).collect(); if ps.free_x { - match scales::free_position_scale(spec.find_scale("pos1"), &layer_dfs, "pos1") { + match scales::free_position_scale(spec.find_scale("pos1"), panel.index) { Some(hs) => view.insert_scale(ps.pos1.clone(), hs), // An empty cell has no extent to free the dimension over, so // fall back to the shared scale rather than leave the axis and @@ -186,7 +184,7 @@ pub fn build_composition( } } if ps.free_y { - match scales::free_position_scale(spec.find_scale("pos2"), &layer_dfs, "pos2") { + match scales::free_position_scale(spec.find_scale("pos2"), panel.index) { Some(hs) => view.insert_scale(ps.pos2.clone(), hs), None => ps.use_shared("pos2"), } diff --git a/src/writer/hephaestus/facet.rs b/src/writer/hephaestus/facet.rs index baf5733af..f15aae467 100644 --- a/src/writer/hephaestus/facet.rs +++ b/src/writer/hephaestus/facet.rs @@ -7,49 +7,33 @@ //! panels plus a [`Panel`] list the writer loops over — one hephaestus `Plot` per //! panel, sharing the composition's scale registry. //! -//! Panel ordering mirrors the Vega-Lite writer's `resolve_facet_ordering`: the -//! facet aesthetic's `SCALE` (its `input_range`, then `reverse`) drives the -//! order, falling back to a numeric-aware ascending sort of the present values. +//! Panel ordering and row assignment are core's, from +//! [`crate::plot::facet::panels`] — the same order per-panel scale resolutions +//! ([`Scale::panels`](crate::plot::Scale::panels)) are stored in, so +//! `Panel::index` indexes directly into them. -use std::cmp::Ordering; -use std::collections::HashSet; - -use arrow::array::{Array, UInt32Array}; use hephaestus::composition::{grid, spacer, Composition, Element, Patch}; -use super::channels::{column_to_f64, column_to_strings}; -use crate::naming; -use crate::plot::{ArrayElement, FacetLayout, ParameterValue, Scale, ScaleTypeKind}; +use crate::plot::facet::{self as facet_panels, FacetLevel}; +use crate::plot::{ArrayElement, FacetLayout, ParameterValue, Scale}; use crate::{DataFrame, Plot, Result}; /// Patch id for the single (unfaceted) panel. pub const PANEL_ID: &str = "ggsql_panel"; -/// The value selecting one facet cell's rows: the facet column's text form, -/// paired with whether the cell was NULL. -/// -/// [`column_to_strings`] renders a NULL as `""`, so text alone would make a -/// genuine empty-string category and a NULL the same panel. The Vega-Lite -/// writer keeps them apart (a null level labels as `"null"` and gets its own -/// facet), so this writer carries the null flag alongside the text everywhere a -/// level is identified or matched. -#[derive(Clone, PartialEq, Eq, Hash)] -pub struct LevelKey { - text: String, - is_null: bool, -} - /// One facet cell: which facet values it holds, its grid position (for edge-only /// axes), and the strip-label text to show. pub struct Panel { /// hephaestus patch id, unique per panel. pub id: String, - /// 0-based panel index (order of enumeration), for per-panel scale names. + /// 0-based panel index (the canonical order, see + /// [`crate::plot::facet::panels`]), indexing [`Scale::panels`] and used for + /// per-panel scale names. pub index: usize, /// Facet1 (Wrap panel / Grid row) value selecting this panel's rows. - pub facet1: Option, + pub facet1: Option, /// Facet2 (Grid column) value; `None` for Wrap. - pub facet2: Option, + pub facet2: Option, /// Top strip label (Wrap header, or Grid column header on the top row). pub strip_top: Option, /// Right strip label (Grid row header on the right column). @@ -112,7 +96,7 @@ fn build_wrap( facet: &crate::plot::Facet, layer0: &DataFrame, ) -> Result<(Composition, Vec)> { - let levels = ordered_levels(spec, layer0, "facet1")?; + let levels = ordered_labelled_levels(spec, layer0, "facet1")?; if levels.is_empty() { return Ok(single_panel()); } @@ -121,7 +105,7 @@ fn build_wrap( let nrow = n.div_ceil(ncol); let mut panels = Vec::with_capacity(n); - for (idx, level) in levels.iter().enumerate() { + for (idx, (level, label)) in levels.iter().enumerate() { let col = idx % ncol; // Bottom-most present panel in this column: no panel sits `ncol` cells // below it. Governs where the x-axis shows when the last row is partial. @@ -131,7 +115,7 @@ fn build_wrap( index: idx, facet1: Some(level.key.clone()), facet2: None, - strip_top: Some(level.label.clone()), + strip_top: Some(label.clone()), strip_right: None, first_col: col == 0, last_row, @@ -153,8 +137,8 @@ fn build_wrap( /// Grid: rows = facet1 levels, columns = facet2 levels. Column strips on the top /// row, row strips on the right column. fn build_grid(spec: &Plot, layer0: &DataFrame) -> Result<(Composition, Vec)> { - let rows = ordered_levels(spec, layer0, "facet1")?; - let cols = ordered_levels(spec, layer0, "facet2")?; + let rows = ordered_labelled_levels(spec, layer0, "facet1")?; + let cols = ordered_labelled_levels(spec, layer0, "facet2")?; if rows.is_empty() || cols.is_empty() { return Ok(single_panel()); } @@ -164,16 +148,16 @@ fn build_grid(spec: &Plot, layer0: &DataFrame) -> Result<(Composition, Vec = Vec::with_capacity(nrow * ncol); let mut index = 0; - for (r, rowv) in rows.iter().enumerate() { - for (c, colv) in cols.iter().enumerate() { + for (r, (rowv, row_label)) in rows.iter().enumerate() { + for (c, (colv, col_label)) in cols.iter().enumerate() { let id = format!("facet_{r}_{c}"); panels.push(Panel { id: id.clone(), index, facet1: Some(rowv.key.clone()), facet2: Some(colv.key.clone()), - strip_top: (r == 0).then(|| colv.label.clone()), - strip_right: (c == ncol - 1).then(|| rowv.label.clone()), + strip_top: (r == 0).then(|| col_label.clone()), + strip_right: (c == ncol - 1).then(|| row_label.clone()), first_col: c == 0, last_row: r == nrow - 1, }); @@ -193,130 +177,29 @@ fn wrap_ncol(facet: &crate::plot::Facet, n: usize) -> usize { } } -/// One distinct facet level: the key selecting its rows, the numeric form of the -/// same cell (the bin-join key for a binned facet), and the strip text. -struct Level { - key: LevelKey, - value: f64, - label: String, -} - -/// The per-row facet key of one facet column: its text form paired with the null -/// bitmap, so a NULL cell selects a different panel than an empty-string -/// category — see [`LevelKey`]. -fn level_keys(df: &DataFrame, column: &str) -> Result> { - let array = df.column(column)?; - Ok(column_to_strings(df, column)? +/// Distinct facet levels in the canonical core order, each paired with its +/// strip-label text. +fn ordered_labelled_levels( + spec: &Plot, + df: &DataFrame, + internal_aes: &str, +) -> Result> { + let scale = spec.find_scale(internal_aes); + Ok(facet_panels::ordered_levels(spec, df, internal_aes)? .into_iter() - .enumerate() - .map(|(i, text)| LevelKey { - text, - is_null: array.is_null(i), + .map(|level| { + let label = facet_label(scale, &level); + (level, label) }) .collect()) } -/// Distinct facet levels present in the data, ordered per the facet scale and -/// labelled for the strip. -fn ordered_levels(spec: &Plot, df: &DataFrame, internal_aes: &str) -> Result> { - let col = naming::aesthetic_column(internal_aes); - let keys = level_keys(df, &col)?; - // The numeric form of the same column, for the binned join. A text facet - // column can't cast; `value` is only read for binned scales. - let values = column_to_f64(df, &col).unwrap_or_else(|_| vec![f64::NAN; keys.len()]); - - let mut seen = HashSet::new(); - let mut distinct: Vec = Vec::new(); - for (i, key) in keys.iter().enumerate() { - if seen.insert(key.clone()) { - distinct.push(Level { - key: key.clone(), - value: values[i], - label: String::new(), - }); - } - } - - let scale = spec.find_scale(internal_aes); - let mut ordered = order_levels(distinct, scale); - for level in &mut ordered { - level.label = facet_label(scale, level); - } - Ok(ordered) -} - -/// Order distinct facet levels, mirroring the Vega-Lite writer's -/// `resolve_facet_ordering`: a binned facet sorts by its (numeric) bin centre; -/// everything else follows the scale's `input_range`, then any present-but-unlisted -/// values sorted numeric-aware ascending. Reversed when the scale sets -/// `reverse => true`. -fn order_levels(mut distinct: Vec, scale: Option<&Scale>) -> Vec { - let reverse = super::scales::is_reversed(scale); - - let mut ordered = if is_binned(scale) { - // Bin centres sort naturally; NULL (censored) panels go last. - distinct.sort_by(|a, b| match (a.value.is_finite(), b.value.is_finite()) { - (true, true) => a.value.total_cmp(&b.value), - (true, false) => Ordering::Less, - (false, true) => Ordering::Greater, - (false, false) => Ordering::Equal, - }); - distinct - } else { - match scale.and_then(|s| s.input_range.as_ref()) { - Some(range) => { - let order: Vec = range.iter().map(element_to_key).collect(); - let (mut ranked, mut extra): (Vec, Vec) = (Vec::new(), Vec::new()); - for key in &order { - if let Some(pos) = distinct.iter().position(|l| &l.key == key) { - ranked.push(distinct.remove(pos)); - } - } - extra.extend(distinct); - sort_levels(&mut extra); - ranked.extend(extra); - ranked - } - None => { - sort_levels(&mut distinct); - distinct - } - } - }; - if reverse { - ordered.reverse(); - } - ordered -} - -/// Numeric-aware ascending sort by key: numeric when every key parses as `f64`, -/// otherwise lexical. -fn sort_levels(levels: &mut [Level]) { - if levels.iter().all(|l| l.key.text.parse::().is_ok()) { - levels.sort_by(|a, b| { - let a = a.key.text.parse::().unwrap(); - let b = b.key.text.parse::().unwrap(); - a.partial_cmp(&b).unwrap_or(Ordering::Equal) - }); - } else { - levels.sort_by(|a, b| a.key.text.cmp(&b.key.text)); - } -} - -/// Whether a facet scale is binned (numeric/temporal facet columns default to it). -fn is_binned(scale: Option<&Scale>) -> bool { - scale - .and_then(|s| s.scale_type.as_ref()) - .map(|st| st.scale_type_kind()) - == Some(ScaleTypeKind::Binned) -} - /// Strip text for one facet level, mirroring the Vega-Lite writer's /// `build_indexed_facet_label_expr` (discrete + `RENAMING`) and /// `build_binned_facet_label_expr` (bin ranges). Computed here from typed values /// rather than as a Vega expression over serialized data, so a temporal binned /// facet — which Vega-Lite silently fails to match — labels correctly. -fn facet_label(scale: Option<&Scale>, level: &Level) -> String { +fn facet_label(scale: Option<&Scale>, level: &FacetLevel) -> String { // NULL keys as the literal string "null", matching ggsql's RENAMING key for // a null level (`RENAMING null => 'The rest'`). if level.key.is_null { @@ -329,7 +212,7 @@ fn facet_label(scale: Option<&Scale>, level: &Level) -> String { None => "null".to_string(), }; } - if is_binned(scale) { + if facet_panels::is_binned(scale) { // The column carries the bin centre; label it with the bin's range. let bins = scale.map(super::scales::binned_bins).unwrap_or_default(); if let Some(i) = super::scales::bin_at_centre(&bins, level.value) { @@ -342,7 +225,7 @@ fn facet_label(scale: Option<&Scale>, level: &Level) -> String { /// A discrete/ordinal level's label: the `RENAMING` override for its domain /// value, an empty strip when suppressed, else the raw value. -fn discrete_label(scale: Option<&Scale>, level: &Level) -> String { +fn discrete_label(scale: Option<&Scale>, level: &FacetLevel) -> String { let Some(scale) = scale else { return level.key.text.clone(); }; @@ -371,8 +254,8 @@ fn discrete_label(scale: Option<&Scale>, level: &Level) -> String { /// Whether a domain element denotes the same value as this level: by key first, /// then numerically (a `DOUBLE` column's `"5.0"` still matches `Number(5.0)`). -fn element_matches(element: &ArrayElement, level: &Level) -> bool { - if element_to_key(element) == level.key { +fn element_matches(element: &ArrayElement, level: &FacetLevel) -> bool { + if facet_panels::element_to_key(element) == level.key { return true; } if level.key.is_null { @@ -384,24 +267,6 @@ fn element_matches(element: &ArrayElement, level: &Level) -> bool { } } -/// An `input_range` element as the key the facet column carries for it: the text -/// form the column casts to (whole numbers as integers, matching an integer -/// column's cast to text), plus whether the element is the null level. -fn element_to_key(element: &ArrayElement) -> LevelKey { - let text = match element { - ArrayElement::String(s) => s.clone(), - ArrayElement::Number(n) if n.fract() == 0.0 && n.is_finite() => format!("{}", *n as i64), - ArrayElement::Number(n) => n.to_string(), - ArrayElement::Boolean(b) => b.to_string(), - ArrayElement::Null => String::new(), - other => format!("{other:?}"), - }; - LevelKey { - text, - is_null: matches!(element, ArrayElement::Null), - } -} - /// The scale names a panel binds its position channels to, and whether each /// dimension is free. For fixed dimensions the name is the shared `pos1`/`pos2`; /// for free dimensions it is a per-panel name (`pos1__p{index}`), so each panel @@ -452,27 +317,13 @@ pub fn panel_dataframe(df: &DataFrame, panel: &Panel) -> Result { let Some(want1) = &panel.facet1 else { return Ok(df.clone()); }; - let f1 = naming::aesthetic_column("facet1"); - if df.column(&f1).is_err() { - return Ok(df.clone()); - } - let c1 = level_keys(df, &f1)?; - let c2 = match &panel.facet2 { - Some(_) => Some(level_keys(df, &naming::aesthetic_column("facet2"))?), - None => None, + let key = facet_panels::PanelKey { + index: panel.index, + facet1: want1.clone(), + facet2: panel.facet2.clone(), }; - - let mut idx: Vec = Vec::new(); - for i in 0..df.height() { - if &c1[i] != want1 { - continue; - } - if let (Some(c2), Some(want2)) = (&c2, &panel.facet2) { - if &c2[i] != want2 { - continue; - } - } - idx.push(i as u32); + match facet_panels::rows_in_panel(df, &key)? { + Some(idx) => df.take(&arrow::array::UInt32Array::from(idx)), + None => Ok(df.clone()), } - df.take(&UInt32Array::from(idx)) } diff --git a/src/writer/hephaestus/scales.rs b/src/writer/hephaestus/scales.rs index 6388a4a42..559512fab 100644 --- a/src/writer/hephaestus/scales.rs +++ b/src/writer/hephaestus/scales.rs @@ -16,12 +16,9 @@ use hephaestus::scales::value::{ }; use hephaestus::scales::Direction; -use super::channels::{column_to_channel, column_to_f64, ChannelData, NULL_CATEGORY}; -use crate::naming; -use crate::plot::aesthetic::POSITION_SUFFIXES; +use super::channels::NULL_CATEGORY; use crate::plot::scale::{linetype_to_stroke_dash, TransformKind as GTransform}; use crate::plot::{ArrayElement, OutputRange, ParameterValue, Scale as GScale, ScaleTypeKind}; -use crate::DataFrame; /// What kind of visual output a scale's range produces. Selects how a resolved /// `OutputRange::Array` is mapped onto a hephaestus range — and, for the values @@ -124,57 +121,111 @@ pub fn build_scale(scale: &GScale, kind: RangeKind) -> Option { Some(apply_breaks(hs, scale, type_kind)) } -/// Build a per-panel position scale for a **free** facet dimension, computing -/// the domain from this panel's own data slices. +/// Build a per-panel position scale for a **free** facet dimension from core's +/// per-panel resolution ([`GScale::panels`]). /// -/// This is a deliberate, scoped exception to ggsql owning all scale domains -/// (fixed dimensions still pass `numeric_domain()` straight through): only free -/// facet dimensions derive a per-panel domain here. Continuous dimensions take -/// the numeric extent of the position family (`pos1`, `pos1min/max/end`, …) -/// present in the slices; discrete/ordinal take the panel's distinct categories; -/// binned dimensions keep ggsql's global bin edges, narrowed to the bins the panel -/// occupies (see [`free_binned_scale`]). ggsql's resolved *continuous* breaks are -/// for the global domain and don't fit a per-panel one, so those ticks are left to -/// hephaestus. -/// -/// The *padding* around a computed extent is still ggsql's: -/// [`Scale::expand_range`](crate::plot::Scale::expand_range) applies the scale's -/// own resolved `expand` factors, so a free panel is padded exactly like a fixed -/// axis. Only the extent is derived here, never the expansion policy. -pub fn free_position_scale( - global: Option<&GScale>, - dfs: &[&DataFrame], - base: &str, -) -> Option { - let type_kind = global - .and_then(|s| s.scale_type.as_ref()) +/// Core resolves the per-panel domain, breaks, labels, and minor breaks with +/// the same machinery as the global scale, so this writer only translates — +/// it never derives a positional extent itself. `None` when core resolved no +/// per-panel entry (an empty facet cell), sending the panel back to the shared +/// scale (`PanelScales::use_shared`). +pub fn free_position_scale(global: Option<&GScale>, panel_index: usize) -> Option { + let g = global?; + let panel = g.panels.as_ref()?.get(panel_index)?.as_ref()?; + let type_kind = g + .scale_type + .as_ref() .map(|st| st.scale_type_kind()) .unwrap_or(ScaleTypeKind::Continuous); - let transform = global - .and_then(|s| s.transform.as_ref()) - .map(|t| t.transform_kind()); + let transform = g.transform.as_ref().map(|t| t.transform_kind()); let hs = match type_kind { ScaleTypeKind::Discrete | ScaleTypeKind::Ordinal => { - let vals = panel_categories(global, dfs, base); - // An empty cell has no categories to free the dimension over; `None` - // sends the panel back to the shared scale (`PanelScales::use_shared`) - // rather than registering a domainless axis. + let vals: Vec = panel.input_range.iter().map(category_value).collect(); + // An empty cell has no categories to free the dimension over. if vals.is_empty() { return None; } - if matches!(type_kind, ScaleTypeKind::Ordinal) { + let mut hs = if matches!(type_kind, ScaleTypeKind::Ordinal) { scale::ordinal(vals) } else { scale::discrete(vals) + }; + // Pair each label with the category at its resolved position — the + // 1-based index into the panel's domain, exactly as the fixed path + // (`apply_breaks`) pairs against the global domain. A suppressed + // label blanks the text but keeps the category. + let pairs: Vec<(HValue, String)> = panel + .breaks + .iter() + .filter_map(|(pos, label)| { + let index = (pos.round() as usize).checked_sub(1)?; + Some(( + category_value(panel.input_range.get(index)?), + label.clone().unwrap_or_default(), + )) + }) + .collect(); + if !pairs.is_empty() { + hs = hs.with_breaks_labeled(pairs); } + hs } ScaleTypeKind::Identity => scale::identity(), - ScaleTypeKind::Binned => global - .and_then(|g| free_binned_scale(g, dfs, base)) - // No usable break array → fall back to a plain continuous panel scale. - .or_else(|| free_continuous_scale(global, dfs, base, transform))?, - ScaleTypeKind::Continuous => free_continuous_scale(global, dfs, base, transform)?, + ScaleTypeKind::Binned => { + // The panel's domain is the narrowed bin-edge window and its breaks + // are those edges — a hephaestus binned scale derives band width + // from its edge count, so the two must narrow together. + let (min, max) = panel_range(&panel.input_range)?; + let edges: Vec = panel.breaks.iter().map(|(pos, _)| *pos).collect(); + let edges = if edges.len() >= 2 { + edges + } else { + vec![min, max] + }; + let mut hs = scale::binned(min..=max, edges); + if let Some(t) = transform.and_then(map_transform) { + hs = hs.with_transform(t); + } + let labels = visible_numbered_labels(&panel.breaks, None); + if !labels.is_empty() { + hs = hs.with_breaks_labeled(labels); + } + hs + } + ScaleTypeKind::Continuous => { + // The panel domain arrives already expanded and transform-clipped — + // core ran the same resolve as for a fixed axis. + let (min, max) = panel_range(&panel.input_range)?; + let (min, max) = pad_degenerate(min, max); + let labels = visible_numbered_labels(&panel.breaks, transform); + match temporal_scale(transform, min, max) { + Some(hs) => { + if labels.is_empty() { + // Leave the tick set automatic when core resolved no + // breaks; pinning minors around ticks hephaestus chose + // itself would mix two grids. + hs + } else { + apply_pinned_minors( + hs.with_breaks_labeled(labels), + panel.minor_breaks.as_deref(), + transform, + ) + } + } + None => { + let mut c = scale::continuous(min..=max); + if let Some(t) = transform.and_then(map_transform) { + c = c.with_transform(t); + } + if !labels.is_empty() { + c = c.with_breaks_labeled(labels); + } + apply_pinned_minors(c, panel.minor_breaks.as_deref(), transform) + } + } + } }; // The same flag the fixed path sets (see [`build_scale`]): freeing a @@ -186,67 +237,27 @@ pub fn free_position_scale( }) } -/// A per-panel continuous position scale over the panel's own data extent. -/// -/// A temporal dimension becomes a temporal scale, and keeps ggsql's global break -/// labels narrowed to the panel — the same treatment [`free_binned_scale`] gives -/// bin edges, and what the Vega-Lite writer does with a free temporal axis. The -/// alternative, letting hephaestus pick per-panel calendar ticks, invents breaks -/// ggsql didn't resolve and packs full ISO labels into a panel too narrow to hold -/// them (the writer does no label thinning). A panel no global break -/// falls inside keeps hephaestus's own ticks rather than a bare axis; they are -/// dates either way, because the scale carries the calendar unit. -fn free_continuous_scale( - global: Option<&GScale>, - dfs: &[&DataFrame], - base: &str, +/// A panel domain's `(min, max)` from its two endpoint elements. +fn panel_range(input_range: &[crate::plot::ArrayElement]) -> Option<(f64, f64)> { + let min = input_range.first()?.to_f64()?; + let max = input_range.last()?.to_f64()?; + Some((min, max)) +} + +/// A panel's visible breaks (suppressed labels dropped) as hephaestus +/// `(value, label)` pairs, wrapping positions in the transform's value variant. +fn visible_numbered_labels( + breaks: &[(f64, Option)], transform: Option, -) -> Option { - let (min, max) = panel_extent(dfs, base)?; - // A panel extent is raw data, where `numeric_domain()` would already be - // expanded, so pad it with the scale's own resolved expansion — otherwise a - // free panel's marks sit hard against the panel edge while a fixed axis gets - // 5%, and `SETTING expand` silently stops applying once a dimension is freed. - let (min, max) = match global { - Some(g) => g.expand_range(min, max), - None => (min, max), - }; - let (min, max) = pad_degenerate(min, max); - // ggsql's global minors, narrowed to this panel — the same treatment its majors - // get below. Pinning these is what keeps a panel showing one major from being - // filled with hephaestus's own sub-unit minors: ggsql derives minors from the - // global major spacing, so the survivors stay on that grid. `None` (no minors - // resolved) stays None so the fallback survives; an empty list after filtering is - // a panel that genuinely contains none. - let minors: Option> = global - .and_then(|g| g.numeric_minor_breaks()) - .map(|positions| { - positions - .into_iter() - .filter(|pos| *pos >= min && *pos <= max) - .collect() - }); - if let Some(hs) = temporal_scale(transform, min, max) { - let labels: Vec<(HValue, String)> = global - .map(|g| g.break_labels()) - .unwrap_or_default() - .into_iter() - .filter(|(pos, _)| *pos >= min && *pos <= max) - .map(|(pos, label)| (temporal_value(transform, pos), label)) - .collect(); - // Leave the whole tick set automatic when no global break lands in the panel; - // pinning minors around ticks hephaestus chose itself would mix two grids. - return Some(if labels.is_empty() { - hs - } else { - apply_pinned_minors(hs.with_breaks_labeled(labels), minors.as_deref(), transform) - }); - } - let mut c = scale::continuous(min..=max); - if let Some(t) = transform.and_then(map_transform) { - c = c.with_transform(t); - } - Some(apply_pinned_minors(c, minors.as_deref(), transform)) +) -> Vec<(HValue, String)> { + breaks + .iter() + .filter_map(|(pos, label)| { + label + .clone() + .map(|label| (temporal_value(transform, *pos), label)) + }) + .collect() } /// Pin `minors` (ggsql positions, already narrowed to the target domain) on `hs`, @@ -272,127 +283,6 @@ fn apply_pinned_minors( } } -/// A per-panel **binned** position scale: ggsql's globally resolved bin edges, -/// narrowed to the window of bins this panel's data occupies. -/// -/// The writer never invents bin boundaries — it only selects from the edges ggsql -/// resolved, and labels them with ggsql's own edge labels. Edges and domain narrow -/// together because a hephaestus binned scale derives band width from its edge -/// count as `1 / (edges - 1)`: keeping every global edge while shrinking the domain -/// would leave each bar a global bin-width wide, hanging off the panel. -/// -/// Neither `expand_range` nor pinned minors here: the band width a bar is drawn at -/// assumes the domain spans exactly the edges, so padding the domain would -/// desynchronise bar width from bin width, and a binned axis's ticks are its edges, -/// with nothing to subdivide. -fn free_binned_scale(global: &GScale, dfs: &[&DataFrame], base: &str) -> Option { - let bins = binned_bins(global); - if bins.is_empty() { - return None; - } - let (lo, hi) = panel_extent(dfs, base)?; - // The inclusive window of bins covering the panel's extent. - let first = bins.iter().rposition(|b| b.lower <= lo).unwrap_or(0); - let last = bins - .iter() - .position(|b| b.upper >= hi) - .unwrap_or(bins.len() - 1); - let (first, last) = (first.min(last), last); - let window = &bins[first..=last]; - - let mut edges = Vec::with_capacity(window.len() + 1); - edges.push(window[0].lower); - edges.extend(window.iter().map(|b| b.upper)); - let mut hs = scale::binned(window[0].lower..=window[window.len() - 1].upper, edges); - if let Some(t) = global - .transform - .as_ref() - .map(|t| t.transform_kind()) - .and_then(map_transform) - { - hs = hs.with_transform(t); - } - // ggsql's edge labels, restricted to the edges this panel's window keeps. - let labels: Vec<(HValue, String)> = global - .break_labels() - .into_iter() - .filter(|(pos, _)| *pos >= window[0].lower && *pos <= window[window.len() - 1].upper) - .map(|(pos, label)| (HValue::Number(pos), label)) - .collect(); - Some(if labels.is_empty() { - hs - } else { - hs.with_breaks_labeled(labels) - }) -} - -/// The finite numeric extent of a position family across the given slices. -fn panel_extent(dfs: &[&DataFrame], base: &str) -> Option<(f64, f64)> { - let mut lo = f64::INFINITY; - let mut hi = f64::NEG_INFINITY; - for df in dfs { - // The base aesthetic plus its whole position family, so a panel holding - // only extents (a bar's `pos2end`, a ribbon's `pos2min`/`max`) still - // sizes its axis — the same family `execute/scale.rs` trains a fixed - // scale over. - for suffix in std::iter::once("").chain(POSITION_SUFFIXES.iter().copied()) { - let name = naming::aesthetic_column(&format!("{base}{suffix}")); - if df.column(&name).is_ok() { - if let Ok(values) = column_to_f64(df, &name) { - for v in values.into_iter().filter(|v| v.is_finite()) { - lo = lo.min(v); - hi = hi.max(v); - } - } - } - } - } - (lo.is_finite() && hi.is_finite()).then_some((lo, hi)) -} - -/// The categories a panel occupies: ggsql's globally resolved domain, narrowed -/// to the levels these slices actually contain and left in the global order. -/// -/// Selecting from `input_range` is what keeps a free panel agreeing with a fixed -/// one — the same level order, the same [`channels::NULL_CATEGORY`] sentinel for -/// a null level, and the same value *type* [`column_to_channel`] hands over. -/// Re-deriving the domain from the column's text would break all three. The same -/// narrowing [`free_binned_scale`] does for bin edges. -fn panel_categories(global: Option<&GScale>, dfs: &[&DataFrame], base: &str) -> Vec { - let name = naming::aesthetic_column(base); - let domain = domain_values(global); - let mut present = vec![false; domain.len()]; - for df in dfs { - let Ok(data) = column_to_channel(df, &name) else { - continue; - }; - // Matched with `key_eq`, exactly as hephaestus matches data to domain at - // draw time, so a level counts as present here only if it would resolve - // there too. - for value in channel_values(data) { - if let Some(i) = domain.iter().position(|level| level.key_eq(&value)) { - present[i] = true; - } - } - } - domain - .into_iter() - .zip(present) - .filter_map(|(level, present)| present.then_some(level)) - .collect() -} - -/// A column's values as the hephaestus values a scale domain is matched against. -fn channel_values(data: ChannelData) -> Vec { - match data { - ChannelData::Strings(values) => values - .into_iter() - .map(|v| HValue::String(Arc::from(v.as_str()))) - .collect(), - ChannelData::Floats(values) => values.into_iter().map(HValue::Number).collect(), - } -} - /// Domain for a continuous scale. ggsql's resolved `numeric_domain` is /// authoritative — it carries ggsql's global, expanded, transform-aware training /// over every layer and the whole position family — so pass it straight through, @@ -599,7 +489,11 @@ fn apply_minor_breaks(hs: HScale, scale: &GScale, type_kind: ScaleTypeKind) -> H /// value a binned data column actually carries — see `Binned::pre_stat_transform_sql`), /// and its display label. pub struct Bin { + /// Kept for completeness of the bin representation; the writer currently + /// only joins on `centre` and reads `label`. + #[allow(dead_code)] pub lower: f64, + #[allow(dead_code)] pub upper: f64, pub centre: f64, pub label: String, @@ -745,6 +639,7 @@ fn pad_degenerate(min: f64, max: f64) -> (f64, f64) { #[cfg(test)] mod tests { use super::*; + use crate::writer::hephaestus::channels::{column_to_channel, ChannelData}; use std::collections::HashMap; /// A binned scale with the given edges, plus optional per-edge label overrides diff --git a/src/writer/vegalite/encoding.rs b/src/writer/vegalite/encoding.rs index 5a1e18234..6e5863d7b 100644 --- a/src/writer/vegalite/encoding.rs +++ b/src/writer/vegalite/encoding.rs @@ -662,6 +662,16 @@ fn apply_breaks_to_encoding( ) { use crate::plot::ParameterValue; + // A free facet dimension delegates its domain to Vega-Lite per panel + // (`resolve.scale: independent`, no `domain` emitted), so pinning the + // globally resolved break set would leave each panel showing only + // whichever global breaks happen to fall inside it — frequently one or + // none. Vega-Lite cannot consume per-panel axis values, so ticks are left + // to Vega's own computation over each panel's independent domain. + if is_position_aesthetic(aesthetic) && is_free(aesthetic, spec.facet.as_ref()) { + return; + } + let Some(ParameterValue::Array(breaks)) = scale.properties.get("breaks") else { return; }; @@ -1374,6 +1384,7 @@ mod tests { resolved: false, label_mapping: None, label_template: "{}".to_string(), + panels: None, } } diff --git a/src/writer/vegalite/layer.rs b/src/writer/vegalite/layer.rs index 91d4bfce9..0a19fb837 100644 --- a/src/writer/vegalite/layer.rs +++ b/src/writer/vegalite/layer.rs @@ -4311,6 +4311,7 @@ mod tests { resolved: false, label_mapping: None, label_template: "{}".to_string(), + panels: None, }]; let context = RenderContext::new( &scales, @@ -4344,6 +4345,7 @@ mod tests { resolved: false, label_mapping: None, label_template: "{}".to_string(), + panels: None, }]; let context = RenderContext::new( &scales, @@ -4373,6 +4375,7 @@ mod tests { resolved: false, label_mapping: None, label_template: "{}".to_string(), + panels: None, }]; let context = RenderContext::new( &scales, diff --git a/src/writer/vegalite/mod.rs b/src/writer/vegalite/mod.rs index 9162da66a..9671f6da7 100644 --- a/src/writer/vegalite/mod.rs +++ b/src/writer/vegalite/mod.rs @@ -2871,6 +2871,104 @@ mod tests { ); } + #[test] + fn test_facet_free_scales_omits_axis_values() { + // A free facet dimension delegates its domain to Vega-Lite per panel, + // so pinning the globally resolved breaks as axis.values would leave + // each panel showing only the global breaks that happen to fall inside + // it (issue #516). Ticks must be left to Vega's per-panel computation. + use crate::plot::scale::Scale; + use crate::plot::{ArrayElement, Facet, FacetLayout, ParameterValue}; + + let writer = VegaLiteWriter::new(); + + let mut spec = Plot::new(); + let layer = Layer::new(Geom::point()) + .with_aesthetic( + "x".to_string(), + AestheticValue::standard_column("x".to_string()), + ) + .with_aesthetic( + "y".to_string(), + AestheticValue::standard_column("y".to_string()), + ); + spec.layers.push(layer); + + // free => [true, false]: only x is free + let mut facet_properties = Parameters::new(); + facet_properties.insert( + "free".to_string(), + ParameterValue::Array(vec![ + ArrayElement::Boolean(true), + ArrayElement::Boolean(false), + ]), + ); + spec.facet = Some(Facet { + layout: FacetLayout::Wrap { + variables: vec!["category".to_string()], + }, + properties: facet_properties, + resolved: true, + }); + + // Resolved breaks on both positional scales. + let mut x_scale = Scale::new("x"); + x_scale.input_range = Some(vec![ArrayElement::Number(0.0), ArrayElement::Number(100.0)]); + x_scale.properties.insert( + "breaks".to_string(), + ParameterValue::Array(vec![ + ArrayElement::Number(0.0), + ArrayElement::Number(50.0), + ArrayElement::Number(100.0), + ]), + ); + spec.scales.push(x_scale); + let mut y_scale = Scale::new("y"); + y_scale.input_range = Some(vec![ArrayElement::Number(0.0), ArrayElement::Number(10.0)]); + y_scale.properties.insert( + "breaks".to_string(), + ParameterValue::Array(vec![ + ArrayElement::Number(0.0), + ArrayElement::Number(5.0), + ArrayElement::Number(10.0), + ]), + ); + spec.scales.push(y_scale); + + let df = df! { + "x" => vec![1, 2, 3], + "y" => vec![4, 5, 6], + "category" => vec!["A", "A", "B"], + "__ggsql_aes_facet1__" => vec!["A", "A", "B"], + } + .unwrap(); + + transform_spec(&mut spec); + let json_str = writer.write(&spec, &wrap_data(df)).unwrap(); + assert_valid_vegalite(&json_str); + let vl_spec: Value = serde_json::from_str(&json_str).unwrap(); + + // The free dimension must NOT pin axis.values; the fixed one must. + let x_encoding = &vl_spec["spec"]["layer"][0]["encoding"]["x"]; + assert!( + x_encoding + .get("axis") + .and_then(|a| a.get("values")) + .is_none(), + "free x should NOT pin axis.values, got: {}", + serde_json::to_string_pretty(&x_encoding).unwrap() + ); + let y_encoding = &vl_spec["spec"]["layer"][0]["encoding"]["y"]; + assert!( + y_encoding + .get("axis") + .and_then(|a| a.get("values")) + .is_some(), + "fixed y should still pin axis.values, got: {}", + serde_json::to_string_pretty(&y_encoding).unwrap() + ); + } + #[test] fn test_facet_free_y_only_omits_y_domain() { // Test that FACET with free => [false, true] omits y domain but keeps x domain