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

[Bug] fix file handler row parsing to support single geojson feature #1212

Merged
merged 1 commit into from
Jul 31, 2020

Conversation

heshan0131
Copy link
Contributor

#1196
Signed-off-by: Shan He heshan0131@gmail.com

Signed-off-by: Shan He <heshan0131@gmail.com>
@heshan0131 heshan0131 requested a review from ibgreen July 31, 2020 01:13
Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

I see you were able to put the JSONPath helper to good use.

@@ -103,6 +103,7 @@ export async function* readBatch(asyncIterator, fileName) {
for await (const batch of asyncIterator) {
// Last batch will have this special type and will provide all the root
// properties of the parsed document.
// Only json parse will have `FINAL_RESULT`
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably not rely on this as a way of detecting json parsing, in the future some other loader might also emit a FINAL_RESULT batch.

But the check for jsonpath should be robust enough.

// The streamed object is a ROW JSON-batch
} else if (batch.jsonpath && batch.jsonpath.length === 1) {
// The streamed object is a ROW JSON-batch (jsonpath = '$')
// row objects
result = batches;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a bit confusing to call this variable batches, because in my mind we are iterating over the batches and accumulating a data array/table.

I would just call this data (or if more elaborate perhaps streamedData or accumulatedData or table etc).

@@ -112,8 +113,9 @@ export async function* readBatch(asyncIterator, fileName) {
if (batch.jsonpath && batch.jsonpath.length > 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is indeed a bit too messy. At minimum, loaders.gl should provide a helper function that takes the final batch and the accumulated data array and stich them together, handling all the corner cases. In preparation, I'd start by breaking out the logic here:

rebuildJSONObject(finalBatch, accumulatedData)

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