Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix date typing in arrow tables #2096

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
"@types/node": "^20.5.0",
"@typescript-eslint/eslint-plugin": "^7.2.0",
"@typescript-eslint/parser": "^7.2.0",
"apache-arrow": "^16.1.0",
"c8": "^9.1.0",
"canvas": "^2.0.0",
"d3-geo-projection": "^4.0.0",
Expand Down
14 changes: 14 additions & 0 deletions src/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,20 @@ export function arrayify(values) {
case "Sphere":
return [values];
}

// Duck type Arrow tables to retype date fields to Dates. Note that we only
// need the first non-nullish value to be typed correctly for isTemporal to
// return true.
const fields = values?.schema?.fields;
if (Array.isArray(fields)) {
values = Array.from(values);
for (const f of fields) {
if (String(f).endsWith("<MILLISECOND>"))
values.some((d, i) => d[f.name] != null && (values[i] = {...values[i], [f.name]: new Date(values[i][f.name])}));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have Date coercion if the scale type is set to temporal. Can we use that instead here? I.e., change the isTemporal test instead of changing arrayify? I think we want to minimize the logic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we could do that. As soon as we apply Array.from to the arrow table, the schema information that we need to distinguish between a column of numbers and a column of dates is lost. The data becomes an Array of Proxy objects, with getters that return numbers (even on date-typed columns, per apache/arrow#40892 — note that this change discusses a future improvement to make the Proxy return Date objects again, but I don't think it has happened yet).

But in fact, applying Array.from is already a losing proposition performance-wise, and we should use the columnar format directly (#2030). In a sense, this PR is only a stop-gap measure. I've ported the necessary changes to #2030 too, if we want to take that leap instead.

}
return values;
}

return Array.from(values);
}

Expand Down
121 changes: 121 additions & 0 deletions test/output/arrowDates.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
9 changes: 9 additions & 0 deletions test/plots/arrow-dates.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import * as Plot from "@observablehq/plot";
import * as Arrow from "apache-arrow";
import * as d3 from "d3";

export async function arrowDates() {
const athletes = await d3.csv<any>("data/athletes.csv", d3.autoType);
const table = Arrow.tableFromJSON(athletes);
return Plot.rectY(table, Plot.binX(undefined, {x: "date_of_birth"})).plot();
}
1 change: 1 addition & 0 deletions test/plots/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export * from "./aapl-volume.js";
export * from "./anscombe-quartet.js";
export * from "./arc.js";
export * from "./armadillo.js";
export * from "./arrow-dates.js";
export * from "./aspectRatio.js";
export * from "./athletes-bins-colors.js";
export * from "./athletes-birthdays.js";
Expand Down
Loading