ARROW-5537: [JS] Support delta dictionaries in RecordBatchWriter and DictionaryBuilder by trxcllnt · Pull Request #4502 · apache/arrow

trxcllnt · 2019-06-09T10:54:44Z

Adds support for building and writing delta dictionaries. Moves the dictionary Vector pointer to the Data class, similar to #4316.

Forked from #4476 since this adds support for delta dictionaries to the DictionaryBuilder. Will rebase this PR after that's merged. All the work is in the last commit, here: b12d842

TheNeuralBit

Looks great, thanks! Just a couple of minor comments.

I'm a little confused about what's going on with the hard coded nullable in Field. It looks like maybe it's something that was changed for testing and then left in?

TheNeuralBit · 2019-06-21T20:21:29Z

 } else if (val && val.type && (values[++valueIndex] = val)) {
 val instanceof Data && (values[valueIndex] = val = Vector.new(val) as Vector);
- fields[++fieldIndex] = Field.new(field, val.type, val.nullCount > 0) as Field<T[keyof T]>;
+ fields[++fieldIndex] = Field.new(field, val.type, true) as Field<T[keyof T]>;


Why are all these checks hard-coded to true now?

@TheNeuralBit Ah yes, I forgot I had to change this. It's not for testing, but for streaming. The problem goes like this:
We start pulling from a source stream and appending values to the builder
The builder reaches the high-water-mark, and yields a RecordBatch to the Writer
The Writer sees it hasn't written a Schema yet, so it uses the Schema of the first batch
If the first RecordBatch doesn't contain nulls, the Schema Message's Field will have nullable=false. If the source later yields a null, changing the Field's nullable=true won't be reflected in the stream since the Schema has already been sent. It's hacky, but I changed the default value of nullable=true to accommodate this unless explicitly set.

Technically nullable doesn't indicate the existence of nulls, just that it's possible there will be nulls. I'm not sure if other implementations infer nullable=true to also mean there's a non-zero-length null bitmap buffer. If so, we can update the writer to synthesize one for compatibility.

Ah I see. The spec says you're allowed to elide validity bitmaps when the null count is 0, so I think we're ok there:
Arrays having a 0 null count may choose to not allocate the null bitmap. Implementations may choose to always allocate one anyway as a matter of convenience, but this should be noted when memory is being shared.
(from http://arrow.apache.org/docs/format/Layout.html#null-bitmaps)
Doesn't need to happen here, but could we set nullable based on the builder options? e.g. it there are no null_values. I guess we would probably need another builder option (like nullable = false) to do that right, since you could still append null or undefined even without null_values.

trxcllnt · 2019-06-09T10:54:44Z

Adds support for building and writing delta dictionaries. Moves the dictionary Vector pointer to the Data class, similar to #4316.

Forked from #4476 since this adds support for delta dictionaries to the DictionaryBuilder. Will rebase this PR after that's merged. All the work is in the last commit, here: b12d842

TheNeuralBit

Looks great, thanks! Just a couple of minor comments.

I'm a little confused about what's going on with the hard coded nullable in Field. It looks like maybe it's something that was changed for testing and then left in?

TheNeuralBit · 2019-06-21T20:21:29Z

 } else if (val && val.type && (values[++valueIndex] = val)) {
 val instanceof Data && (values[valueIndex] = val = Vector.new(val) as Vector);
- fields[++fieldIndex] = Field.new(field, val.type, val.nullCount > 0) as Field<T[keyof T]>;
+ fields[++fieldIndex] = Field.new(field, val.type, true) as Field<T[keyof T]>;


Why are all these checks hard-coded to true now?

@TheNeuralBit Ah yes, I forgot I had to change this. It's not for testing, but for streaming. The problem goes like this:
We start pulling from a source stream and appending values to the builder
The builder reaches the high-water-mark, and yields a RecordBatch to the Writer
The Writer sees it hasn't written a Schema yet, so it uses the Schema of the first batch
If the first RecordBatch doesn't contain nulls, the Schema Message's Field will have nullable=false. If the source later yields a null, changing the Field's nullable=true won't be reflected in the stream since the Schema has already been sent. It's hacky, but I changed the default value of nullable=true to accommodate this unless explicitly set.

Technically nullable doesn't indicate the existence of nulls, just that it's possible there will be nulls. I'm not sure if other implementations infer nullable=true to also mean there's a non-zero-length null bitmap buffer. If so, we can update the writer to synthesize one for compatibility.

Ah I see. The spec says you're allowed to elide validity bitmaps when the null count is 0, so I think we're ok there:
Arrays having a 0 null count may choose to not allocate the null bitmap. Implementations may choose to always allocate one anyway as a matter of convenience, but this should be noted when memory is being shared.
(from http://arrow.apache.org/docs/format/Layout.html#null-bitmaps)
Doesn't need to happen here, but could we set nullable based on the builder options? e.g. it there are no null_values. I guess we would probably need another builder option (like nullable = false) to do that right, since you could still append null or undefined even without null_values.

trxcllnt · 2019-06-09T10:54:44Z

Adds support for building and writing delta dictionaries. Moves the dictionary Vector pointer to the Data class, similar to #4316.

Forked from #4476 since this adds support for delta dictionaries to the DictionaryBuilder. Will rebase this PR after that's merged. All the work is in the last commit, here: b12d842

TheNeuralBit

Looks great, thanks! Just a couple of minor comments.

I'm a little confused about what's going on with the hard coded nullable in Field. It looks like maybe it's something that was changed for testing and then left in?

TheNeuralBit · 2019-06-21T20:21:29Z

 } else if (val && val.type && (values[++valueIndex] = val)) {
 val instanceof Data && (values[valueIndex] = val = Vector.new(val) as Vector);
- fields[++fieldIndex] = Field.new(field, val.type, val.nullCount > 0) as Field<T[keyof T]>;
+ fields[++fieldIndex] = Field.new(field, val.type, true) as Field<T[keyof T]>;


Why are all these checks hard-coded to true now?

@TheNeuralBit Ah yes, I forgot I had to change this. It's not for testing, but for streaming. The problem goes like this:
We start pulling from a source stream and appending values to the builder
The builder reaches the high-water-mark, and yields a RecordBatch to the Writer
The Writer sees it hasn't written a Schema yet, so it uses the Schema of the first batch
If the first RecordBatch doesn't contain nulls, the Schema Message's Field will have nullable=false. If the source later yields a null, changing the Field's nullable=true won't be reflected in the stream since the Schema has already been sent. It's hacky, but I changed the default value of nullable=true to accommodate this unless explicitly set.

Technically nullable doesn't indicate the existence of nulls, just that it's possible there will be nulls. I'm not sure if other implementations infer nullable=true to also mean there's a non-zero-length null bitmap buffer. If so, we can update the writer to synthesize one for compatibility.

Ah I see. The spec says you're allowed to elide validity bitmaps when the null count is 0, so I think we're ok there:
Arrays having a 0 null count may choose to not allocate the null bitmap. Implementations may choose to always allocate one anyway as a matter of convenience, but this should be noted when memory is being shared.
(from http://arrow.apache.org/docs/format/Layout.html#null-bitmaps)
Doesn't need to happen here, but could we set nullable based on the builder options? e.g. it there are no null_values. I guess we would probably need another builder option (like nullable = false) to do that right, since you could still append null or undefined even without null_values.

trxcllnt · 2019-06-09T10:54:44Z

Adds support for building and writing delta dictionaries. Moves the dictionary Vector pointer to the Data class, similar to #4316.

Forked from #4476 since this adds support for delta dictionaries to the DictionaryBuilder. Will rebase this PR after that's merged. All the work is in the last commit, here: b12d842

TheNeuralBit

Looks great, thanks! Just a couple of minor comments.

I'm a little confused about what's going on with the hard coded nullable in Field. It looks like maybe it's something that was changed for testing and then left in?

TheNeuralBit · 2019-06-21T20:21:29Z

 } else if (val && val.type && (values[++valueIndex] = val)) {
 val instanceof Data && (values[valueIndex] = val = Vector.new(val) as Vector);
- fields[++fieldIndex] = Field.new(field, val.type, val.nullCount > 0) as Field<T[keyof T]>;
+ fields[++fieldIndex] = Field.new(field, val.type, true) as Field<T[keyof T]>;


Why are all these checks hard-coded to true now?

@TheNeuralBit Ah yes, I forgot I had to change this. It's not for testing, but for streaming. The problem goes like this:
We start pulling from a source stream and appending values to the builder
The builder reaches the high-water-mark, and yields a RecordBatch to the Writer
The Writer sees it hasn't written a Schema yet, so it uses the Schema of the first batch
If the first RecordBatch doesn't contain nulls, the Schema Message's Field will have nullable=false. If the source later yields a null, changing the Field's nullable=true won't be reflected in the stream since the Schema has already been sent. It's hacky, but I changed the default value of nullable=true to accommodate this unless explicitly set.

Technically nullable doesn't indicate the existence of nulls, just that it's possible there will be nulls. I'm not sure if other implementations infer nullable=true to also mean there's a non-zero-length null bitmap buffer. If so, we can update the writer to synthesize one for compatibility.

Ah I see. The spec says you're allowed to elide validity bitmaps when the null count is 0, so I think we're ok there:
Arrays having a 0 null count may choose to not allocate the null bitmap. Implementations may choose to always allocate one anyway as a matter of convenience, but this should be noted when memory is being shared.
(from http://arrow.apache.org/docs/format/Layout.html#null-bitmaps)
Doesn't need to happen here, but could we set nullable based on the builder options? e.g. it there are no null_values. I guess we would probably need another builder option (like nullable = false) to do that right, since you could still append null or undefined even without null_values.

trxcllnt · 2019-06-09T10:54:44Z

Adds support for building and writing delta dictionaries. Moves the dictionary Vector pointer to the Data class, similar to #4316.

Forked from #4476 since this adds support for delta dictionaries to the DictionaryBuilder. Will rebase this PR after that's merged. All the work is in the last commit, here: b12d842

TheNeuralBit

Looks great, thanks! Just a couple of minor comments.

I'm a little confused about what's going on with the hard coded nullable in Field. It looks like maybe it's something that was changed for testing and then left in?

TheNeuralBit · 2019-06-21T20:21:29Z

 } else if (val && val.type && (values[++valueIndex] = val)) {
 val instanceof Data && (values[valueIndex] = val = Vector.new(val) as Vector);
- fields[++fieldIndex] = Field.new(field, val.type, val.nullCount > 0) as Field<T[keyof T]>;
+ fields[++fieldIndex] = Field.new(field, val.type, true) as Field<T[keyof T]>;


Why are all these checks hard-coded to true now?

@TheNeuralBit Ah yes, I forgot I had to change this. It's not for testing, but for streaming. The problem goes like this:
We start pulling from a source stream and appending values to the builder
The builder reaches the high-water-mark, and yields a RecordBatch to the Writer
The Writer sees it hasn't written a Schema yet, so it uses the Schema of the first batch
If the first RecordBatch doesn't contain nulls, the Schema Message's Field will have nullable=false. If the source later yields a null, changing the Field's nullable=true won't be reflected in the stream since the Schema has already been sent. It's hacky, but I changed the default value of nullable=true to accommodate this unless explicitly set.

Technically nullable doesn't indicate the existence of nulls, just that it's possible there will be nulls. I'm not sure if other implementations infer nullable=true to also mean there's a non-zero-length null bitmap buffer. If so, we can update the writer to synthesize one for compatibility.

Ah I see. The spec says you're allowed to elide validity bitmaps when the null count is 0, so I think we're ok there:
Arrays having a 0 null count may choose to not allocate the null bitmap. Implementations may choose to always allocate one anyway as a matter of convenience, but this should be noted when memory is being shared.
(from http://arrow.apache.org/docs/format/Layout.html#null-bitmaps)
Doesn't need to happen here, but could we set nullable based on the builder options? e.g. it there are no null_values. I guess we would probably need another builder option (like nullable = false) to do that right, since you could still append null or undefined even without null_values.

trxcllnt · 2019-06-09T10:54:44Z

Adds support for building and writing delta dictionaries. Moves the dictionary Vector pointer to the Data class, similar to #4316.

Forked from #4476 since this adds support for delta dictionaries to the DictionaryBuilder. Will rebase this PR after that's merged. All the work is in the last commit, here: b12d842

TheNeuralBit

Looks great, thanks! Just a couple of minor comments.

I'm a little confused about what's going on with the hard coded nullable in Field. It looks like maybe it's something that was changed for testing and then left in?

TheNeuralBit · 2019-06-21T20:21:29Z

 } else if (val && val.type && (values[++valueIndex] = val)) {
 val instanceof Data && (values[valueIndex] = val = Vector.new(val) as Vector);
- fields[++fieldIndex] = Field.new(field, val.type, val.nullCount > 0) as Field<T[keyof T]>;
+ fields[++fieldIndex] = Field.new(field, val.type, true) as Field<T[keyof T]>;


Why are all these checks hard-coded to true now?

@TheNeuralBit Ah yes, I forgot I had to change this. It's not for testing, but for streaming. The problem goes like this:
We start pulling from a source stream and appending values to the builder
The builder reaches the high-water-mark, and yields a RecordBatch to the Writer
The Writer sees it hasn't written a Schema yet, so it uses the Schema of the first batch
If the first RecordBatch doesn't contain nulls, the Schema Message's Field will have nullable=false. If the source later yields a null, changing the Field's nullable=true won't be reflected in the stream since the Schema has already been sent. It's hacky, but I changed the default value of nullable=true to accommodate this unless explicitly set.

Technically nullable doesn't indicate the existence of nulls, just that it's possible there will be nulls. I'm not sure if other implementations infer nullable=true to also mean there's a non-zero-length null bitmap buffer. If so, we can update the writer to synthesize one for compatibility.

Ah I see. The spec says you're allowed to elide validity bitmaps when the null count is 0, so I think we're ok there:
Arrays having a 0 null count may choose to not allocate the null bitmap. Implementations may choose to always allocate one anyway as a matter of convenience, but this should be noted when memory is being shared.
(from http://arrow.apache.org/docs/format/Layout.html#null-bitmaps)
Doesn't need to happen here, but could we set nullable based on the builder options? e.g. it there are no null_values. I guess we would probably need another builder option (like nullable = false) to do that right, since you could still append null or undefined even without null_values.

trxcllnt · 2019-06-09T10:54:44Z

Adds support for building and writing delta dictionaries. Moves the dictionary Vector pointer to the Data class, similar to #4316.

Forked from #4476 since this adds support for delta dictionaries to the DictionaryBuilder. Will rebase this PR after that's merged. All the work is in the last commit, here: b12d842

TheNeuralBit

Looks great, thanks! Just a couple of minor comments.

I'm a little confused about what's going on with the hard coded nullable in Field. It looks like maybe it's something that was changed for testing and then left in?

TheNeuralBit · 2019-06-21T20:21:29Z

 } else if (val && val.type && (values[++valueIndex] = val)) {
 val instanceof Data && (values[valueIndex] = val = Vector.new(val) as Vector);
- fields[++fieldIndex] = Field.new(field, val.type, val.nullCount > 0) as Field<T[keyof T]>;
+ fields[++fieldIndex] = Field.new(field, val.type, true) as Field<T[keyof T]>;


Why are all these checks hard-coded to true now?

@TheNeuralBit Ah yes, I forgot I had to change this. It's not for testing, but for streaming. The problem goes like this:
We start pulling from a source stream and appending values to the builder
The builder reaches the high-water-mark, and yields a RecordBatch to the Writer
The Writer sees it hasn't written a Schema yet, so it uses the Schema of the first batch
If the first RecordBatch doesn't contain nulls, the Schema Message's Field will have nullable=false. If the source later yields a null, changing the Field's nullable=true won't be reflected in the stream since the Schema has already been sent. It's hacky, but I changed the default value of nullable=true to accommodate this unless explicitly set.

Technically nullable doesn't indicate the existence of nulls, just that it's possible there will be nulls. I'm not sure if other implementations infer nullable=true to also mean there's a non-zero-length null bitmap buffer. If so, we can update the writer to synthesize one for compatibility.

Ah I see. The spec says you're allowed to elide validity bitmaps when the null count is 0, so I think we're ok there:
Arrays having a 0 null count may choose to not allocate the null bitmap. Implementations may choose to always allocate one anyway as a matter of convenience, but this should be noted when memory is being shared.
(from http://arrow.apache.org/docs/format/Layout.html#null-bitmaps)
Doesn't need to happen here, but could we set nullable based on the builder options? e.g. it there are no null_values. I guess we would probably need another builder option (like nullable = false) to do that right, since you could still append null or undefined even without null_values.

trxcllnt · 2019-06-09T10:54:44Z

Adds support for building and writing delta dictionaries. Moves the dictionary Vector pointer to the Data class, similar to #4316.

Forked from #4476 since this adds support for delta dictionaries to the DictionaryBuilder. Will rebase this PR after that's merged. All the work is in the last commit, here: b12d842

TheNeuralBit

Looks great, thanks! Just a couple of minor comments.

I'm a little confused about what's going on with the hard coded nullable in Field. It looks like maybe it's something that was changed for testing and then left in?

TheNeuralBit · 2019-06-21T20:21:29Z

 } else if (val && val.type && (values[++valueIndex] = val)) {
 val instanceof Data && (values[valueIndex] = val = Vector.new(val) as Vector);
- fields[++fieldIndex] = Field.new(field, val.type, val.nullCount > 0) as Field<T[keyof T]>;
+ fields[++fieldIndex] = Field.new(field, val.type, true) as Field<T[keyof T]>;


Why are all these checks hard-coded to true now?

@TheNeuralBit Ah yes, I forgot I had to change this. It's not for testing, but for streaming. The problem goes like this:
We start pulling from a source stream and appending values to the builder
The builder reaches the high-water-mark, and yields a RecordBatch to the Writer
The Writer sees it hasn't written a Schema yet, so it uses the Schema of the first batch
If the first RecordBatch doesn't contain nulls, the Schema Message's Field will have nullable=false. If the source later yields a null, changing the Field's nullable=true won't be reflected in the stream since the Schema has already been sent. It's hacky, but I changed the default value of nullable=true to accommodate this unless explicitly set.

Technically nullable doesn't indicate the existence of nulls, just that it's possible there will be nulls. I'm not sure if other implementations infer nullable=true to also mean there's a non-zero-length null bitmap buffer. If so, we can update the writer to synthesize one for compatibility.

Ah I see. The spec says you're allowed to elide validity bitmaps when the null count is 0, so I think we're ok there:
Arrays having a 0 null count may choose to not allocate the null bitmap. Implementations may choose to always allocate one anyway as a matter of convenience, but this should be noted when memory is being shared.
(from http://arrow.apache.org/docs/format/Layout.html#null-bitmaps)
Doesn't need to happen here, but could we set nullable based on the builder options? e.g. it there are no null_values. I guess we would probably need another builder option (like nullable = false) to do that right, since you could still append null or undefined even without null_values.

trxcllnt requested a review from TheNeuralBit June 9, 2019 10:54

fsaintjacques added the Component: JavaScript label Jun 14, 2019

trxcllnt mentioned this pull request Jun 18, 2019
ARROW-5115: [JS] Add Vector Builders and high-level stream primitives #4476
Closed

trxcllnt force-pushed the js/delta-dictionaries branch from 73d8c01 to 4574bb0Compare June 19, 2019 19:04

make dictionarybuilder and recordbatchwriter support delta dictionaries
6a70a25

trxcllnt force-pushed the js/delta-dictionaries branch from 4574bb0 to 6a70a25Compare June 19, 2019 21:38

trxcllnt marked this pull request as ready for review June 19, 2019 21:38

trxcllnt changed the title ~~ARROW-5537: [JS] [WIP] Support delta dictionaries in RecordBatchWriter and DictionaryBuilder~~ARROW-5537: [JS] Support delta dictionaries in RecordBatchWriter and DictionaryBuilderJun 19, 2019

TheNeuralBit reviewed Jun 21, 2019
View reviewed changes

TheNeuralBit closed this in 40fcebeJun 22, 2019

asfimport mentioned this pull request Jun 22, 2019
[JS] Support delta dictionaries in RecordBatchWriter and DictionaryBuilder #21984
Closed

asfimport mentioned this pull request Jun 22, 2019
[JS] Remove hard-coded Field.nullable #22120
Closed