Skip to content

Conversation

@tianchen92
Copy link
Contributor

No description provided.

@github-actions
Copy link

github-actions bot commented Aug 3, 2020

@tianchen92 tianchen92 force-pushed the ARROW-9304 branch 2 times, most recently from 99a2881 to cb10afc Compare August 13, 2020 06:18
@tianchen92 tianchen92 force-pushed the ARROW-9304 branch 3 times, most recently from 0c7ace8 to 7b87f34 Compare August 17, 2020 04:08
@emkornfield
Copy link
Contributor

@pitrou does this seem ok now?

@pitrou
Copy link
Member

pitrou commented Aug 27, 2020

As for naming, according to various dictionaries, "empty" as a noun (and "empties") is rather informal.
It seems AppendEmptyValue and AppendEmptyValues may be more appropriate (and more explicit), but I'm not a native English speaker.

@tianchen92 tianchen92 force-pushed the ARROW-9304 branch 2 times, most recently from 0bd9afb to af087b4 Compare September 8, 2020 02:45
@tianchen92
Copy link
Contributor Author

As for naming, according to various dictionaries, "empty" as a noun (and "empties") is rather informal.
It seems AppendEmptyValue and AppendEmptyValues may be more appropriate (and more explicit), but I'm not a native English speaker.

Fine, fixed.

@tianchen92 tianchen92 force-pushed the ARROW-9304 branch 3 times, most recently from 535980e to 52e96d5 Compare September 8, 2020 04:57
@tianchen92
Copy link
Contributor Author

Hi, @emkornfield , please also take a look at this patch when you have time.
This patch add AppendEmptyValue/AppendEmptyValues used in StructBuilder#AppendNull/AppendNulls, which would set child null bitmap slot to true rather than false( http://arrow.apache.org/docs/format/Columnar.html#struct-layout).
And now this test ArrowReaderWriter#SingleColumnNullableStruct failed, could you please help fix this since you're export in parquet :) Thanks very much! error message as below:

Failed
Unequal at absolute position 2
@@ -1, +1 @@
-{Backward: 0}
+{Backward: 10}
Expected:
-- is_valid:
[
false,
true
]
-- child 0 type: int64
[
0,
10
]
Actual:
-- is_valid:
[
false,
true
]
-- child 0 type: int64
[
null,
0
]
[ FAILED ] ArrowReadWrite.SingleColumnNullableStruct (7 ms)

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. It looks a lot better now! Here are some more comments.

@pitrou
Copy link
Member

pitrou commented Sep 9, 2020

@tianchen92 The Parquet failure is probably a bug in the nested Parquet reader. I've pushed changes to your branch, be careful to pick them up.

@emkornfield
Copy link
Contributor

@tianchen92 sorry I missed the tag. Is it possible the failure could be related to https://issues.apache.org/jira/browse/ARROW-9603?filter=-2

@tianchen92
Copy link
Contributor Author

@pitrou Thanks for your patch and careful review, I updated PR according to the comments above.
@emkornfield doesn't matter :), pitrou helped disabled the failed test and now seems unblocked.

@nealrichardson
Copy link
Member

@tianchen92 do you want to pick this up again? the bug that @emkornfield mentioned has been fixed.

@tianchen92
Copy link
Contributor Author

@emkornfield @nealrichardson @pitrou Sorry for the delay due to National Day Holiday, I reverted parquet related changes and rebased this PR. Seems tests all passed (one unrelated failing). Please help take a look again, thanks!

@tianchen92 tianchen92 requested a review from pitrou October 10, 2020 10:31
@emkornfield
Copy link
Contributor

@pitrou any objections to merging now (I would expect this might conflict with Decimal256 PR).

tianchen92 and others added 8 commits October 22, 2020 14:27
…tBuilder::AppendNull

Change-Id: I2d04e0e102b5c92418fb1a691ca8b9b9f653f9fc
Change-Id: I9183b580f293d74cd7d083f4a3e879c0ae51d70d
Change-Id: I745c2171929d4039d059496e6b80fc4e381cc97b
Add some simple JSON tests
Change-Id: I94b27df7b2db33f8625713818bad7c2055b9b93a
Change-Id: Icf1c2781fc6d738338eea3854d10d4430660e478
Change-Id: I3743b056585fe2a746638c3466b15681dc66e7c5
@pitrou
Copy link
Member

pitrou commented Oct 22, 2020

Working on this.

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Added some fixes and tests. +1.

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.

5 participants