You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "trxcllnt (via GitHub)" <gi...@apache.org> on 2024/06/04 22:14:25 UTC
[PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
trxcllnt opened a new pull request, #41965:
URL: https://github.com/apache/arrow/pull/41965
This PR makes the stream reader/writer support replacement dictionary messages.
Closes #23576
Closes #41683
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41965:
URL: https://github.com/apache/arrow/pull/41965#issuecomment-2148500200
:warning: GitHub issue #23576 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on code in PR #41965:
URL: https://github.com/apache/arrow/pull/41965#discussion_r1630268826
##########
js/src/ipc/writer.ts:
##########
@@ -294,9 +294,9 @@ export class RecordBatchWriter<T extends TypeMap = any> extends ReadableInterop<
if (!prevDictionary || prevDictionary.data[0] !== chunks[0]) {
// * If `index > 0`, then `isDelta` is true.
// * If `index = 0`, then `isDelta` is false, because this is either the initial or a replacement DictionaryMessage.
- chunks.forEach((chunk, index) => this._writeDictionaryBatch(chunk, id, index > 0));
+ for (const [index, chunk] of chunks.entries()) this._writeDictionaryBatch(chunk, id, index > 0);
Review Comment:
Yeah this shouldn't really matter, it's just weird to read.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #41965:
URL: https://github.com/apache/arrow/pull/41965#discussion_r1630280407
##########
js/src/ipc/writer.ts:
##########
@@ -294,9 +294,9 @@ export class RecordBatchWriter<T extends TypeMap = any> extends ReadableInterop<
if (!prevDictionary || prevDictionary.data[0] !== chunks[0]) {
// * If `index > 0`, then `isDelta` is true.
// * If `index = 0`, then `isDelta` is false, because this is either the initial or a replacement DictionaryMessage.
- chunks.forEach((chunk, index) => this._writeDictionaryBatch(chunk, id, index > 0));
+ for (const [index, chunk] of chunks.entries()) this._writeDictionaryBatch(chunk, id, index > 0);
Review Comment:
I prefer actual for loops since you can `break` them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #41965:
URL: https://github.com/apache/arrow/pull/41965#discussion_r1629685248
##########
js/src/ipc/writer.ts:
##########
@@ -294,9 +294,9 @@ export class RecordBatchWriter<T extends TypeMap = any> extends ReadableInterop<
if (!prevDictionary || prevDictionary.data[0] !== chunks[0]) {
// * If `index > 0`, then `isDelta` is true.
// * If `index = 0`, then `isDelta` is false, because this is either the initial or a replacement DictionaryMessage.
- chunks.forEach((chunk, index) => this._writeDictionaryBatch(chunk, id, index > 0));
+ for (const [index, chunk] of chunks.entries()) this._writeDictionaryBatch(chunk, id, index > 0);
Review Comment:
just confirming that this is what you want
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "pohuan (via GitHub)" <gi...@apache.org>.
pohuan commented on PR #41965:
URL: https://github.com/apache/arrow/pull/41965#issuecomment-2174392736
Thank you Paul! It helps a lot. Also, thanks Dominik for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on PR #41965:
URL: https://github.com/apache/arrow/pull/41965#issuecomment-2162038287
Should be around [July 1](https://github.com/apache/arrow/milestone/62)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on code in PR #41965:
URL: https://github.com/apache/arrow/pull/41965#discussion_r1630209304
##########
js/src/ipc/writer.ts:
##########
@@ -294,9 +294,9 @@ export class RecordBatchWriter<T extends TypeMap = any> extends ReadableInterop<
if (!prevDictionary || prevDictionary.data[0] !== chunks[0]) {
// * If `index > 0`, then `isDelta` is true.
// * If `index = 0`, then `isDelta` is false, because this is either the initial or a replacement DictionaryMessage.
- chunks.forEach((chunk, index) => this._writeDictionaryBatch(chunk, id, index > 0));
+ for (const [index, chunk] of chunks.entries()) this._writeDictionaryBatch(chunk, id, index > 0);
Review Comment:
I do think this lint rule is silly, but I'd rather not disable it in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #41965:
URL: https://github.com/apache/arrow/pull/41965#issuecomment-2163509009
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit fd11b7a042f0a020a0cde34e043ad2e77752bca6.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/26141461673) has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "pohuan (via GitHub)" <gi...@apache.org>.
pohuan commented on code in PR #41965:
URL: https://github.com/apache/arrow/pull/41965#discussion_r1635239141
##########
js/src/ipc/writer.ts:
##########
@@ -294,9 +294,9 @@ export class RecordBatchWriter<T extends TypeMap = any> extends ReadableInterop<
if (!prevDictionary || prevDictionary.data[0] !== chunks[0]) {
// * If `index > 0`, then `isDelta` is true.
// * If `index = 0`, then `isDelta` is false, because this is either the initial or a replacement DictionaryMessage.
- chunks.forEach((chunk, index) => this._writeDictionaryBatch(chunk, id, index > 0));
+ for (const [index, chunk] of chunks.entries()) this._writeDictionaryBatch(chunk, id, index > 0);
Review Comment:
So, is there any change left to commit? Would it be merged recently? Thanks for the help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "domoritz (via GitHub)" <gi...@apache.org>.
domoritz commented on code in PR #41965:
URL: https://github.com/apache/arrow/pull/41965#discussion_r1630252199
##########
js/src/ipc/writer.ts:
##########
@@ -294,9 +294,9 @@ export class RecordBatchWriter<T extends TypeMap = any> extends ReadableInterop<
if (!prevDictionary || prevDictionary.data[0] !== chunks[0]) {
// * If `index > 0`, then `isDelta` is true.
// * If `index = 0`, then `isDelta` is false, because this is either the initial or a replacement DictionaryMessage.
- chunks.forEach((chunk, index) => this._writeDictionaryBatch(chunk, id, index > 0));
+ for (const [index, chunk] of chunks.entries()) this._writeDictionaryBatch(chunk, id, index > 0);
Review Comment:
You can disable it for the line/file only as a compromise. Unless the perf difference is too small to matter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on code in PR #41965:
URL: https://github.com/apache/arrow/pull/41965#discussion_r1630361557
##########
js/src/ipc/writer.ts:
##########
@@ -294,9 +294,9 @@ export class RecordBatchWriter<T extends TypeMap = any> extends ReadableInterop<
if (!prevDictionary || prevDictionary.data[0] !== chunks[0]) {
// * If `index > 0`, then `isDelta` is true.
// * If `index = 0`, then `isDelta` is false, because this is either the initial or a replacement DictionaryMessage.
- chunks.forEach((chunk, index) => this._writeDictionaryBatch(chunk, id, index > 0));
+ for (const [index, chunk] of chunks.entries()) this._writeDictionaryBatch(chunk, id, index > 0);
Review Comment:
And I prefer `forEach()` because you can step over/out of them :stuck_out_tongue:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt merged PR #41965:
URL: https://github.com/apache/arrow/pull/41965
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "pohuan (via GitHub)" <gi...@apache.org>.
pohuan commented on PR #41965:
URL: https://github.com/apache/arrow/pull/41965#issuecomment-2161803486
Hi @trxcllnt thanks for help on that. May I ask you when will 17.0.0 that contains this to be released?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-23576: [JS] Support DictionaryMessage replacement [arrow]
Posted by "trxcllnt (via GitHub)" <gi...@apache.org>.
trxcllnt commented on code in PR #41965:
URL: https://github.com/apache/arrow/pull/41965#discussion_r1630361557
##########
js/src/ipc/writer.ts:
##########
@@ -294,9 +294,9 @@ export class RecordBatchWriter<T extends TypeMap = any> extends ReadableInterop<
if (!prevDictionary || prevDictionary.data[0] !== chunks[0]) {
// * If `index > 0`, then `isDelta` is true.
// * If `index = 0`, then `isDelta` is false, because this is either the initial or a replacement DictionaryMessage.
- chunks.forEach((chunk, index) => this._writeDictionaryBatch(chunk, id, index > 0));
+ for (const [index, chunk] of chunks.entries()) this._writeDictionaryBatch(chunk, id, index > 0);
Review Comment:
I prefer `forEach()` because you can step over/out of them :stuck_out_tongue:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org