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

fix date typing in arrow tables #2096

wants to merge 2 commits into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jun 21, 2024

uses the same duck type test as observablehq/inputs#263

addresses observablehq/framework#1376 (for the Plot part)

Note: it's not as good as full Arrow support (where the data would not get transformed into an array of Proxy objects, and valueof would use table.getChild, see #2030).

package.json Outdated Show resolved Hide resolved
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.

Fil added a commit that referenced this pull request Jun 26, 2024
Fil added a commit that referenced this pull request Jun 26, 2024
@mbostock
Copy link
Member

I incorporated this into #2115:

plot/src/options.js

Lines 67 to 73 in 4faebbc

function maybeTypedArrowify(vector, type) {
return vector == null
? vector
: (type === undefined || type === Array) && isArrowDateType(vector.type)
? coerceDates(vector.toArray())
: maybeTypedArrayify(vector.toArray(), type);
}

I think it would be slightly better in the future to avoid materializing the values as Date instances, and instead somehow retain a hint on the column that the numbers represent dates (like Apache Arrow does with vector.type). But that’s “just” an optimization that we can consider in the future.

@Fil
Copy link
Contributor Author

Fil commented Aug 5, 2024

superseded by #2030

@Fil Fil closed this Aug 5, 2024
mbostock added a commit that referenced this pull request Aug 5, 2024
* columnar support for arrow tables

* defer reading the values until they're actually requested (which is often not the case)

* tests

* comment

* fix apache arrow dates (alternative to #2096)

* add test snapshot

* fix test wrt apache/arrow#40718

* arrow table data; fix BigInt coercion

* more arrow support

* arrow date hint; fix BigInt coercion

* inline floater

* shorten slightly

* valueof tests; better arrow coercion

* Arrow-aware stack transform

* a few more dataify

* fix merge conflict

* fix Plot.find and stack customOrder

* handle Arrow in a few more places

---------

Co-authored-by: Philippe Rivière <[email protected]>
@Fil Fil deleted the fil/arrow-dates branch August 5, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants