Re: [PR] feat: Add missing type to `Struct` [arrow-js]
trxcllnt commented on code in PR #340:
URL: https://github.com/apache/arrow-js/pull/340#discussion_r2560933326
##
src/builder/struct.ts:
##
@@ -21,9 +21,23 @@ import { Field } from '../schema.js';
import { Builder } from '../builder.js';
import { Struct, TypeMap } from '../type.js';
+/** @ignore */
+type StructValue = Struct['TValue'] | { [P in
keyof T]: T[P]['TValue'] };
Review Comment:
Is this needed because of a Typescript version bump? This used to work
because the mapping [is part
of](https://github.com/apache/arrow-js/blob/6bb1bb93e7df5c795a65d5e6ce2469fad758d655/src/row/struct.ts#L28-L30)
the `Struct['TValue']` aka `StructRowProxy`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
trxcllnt commented on code in PR #340:
URL: https://github.com/apache/arrow-js/pull/340#discussion_r2560933326
##
src/builder/struct.ts:
##
@@ -21,9 +21,23 @@ import { Field } from '../schema.js';
import { Builder } from '../builder.js';
import { Struct, TypeMap } from '../type.js';
+/** @ignore */
+type StructValue = Struct['TValue'] | { [P in
keyof T]: T[P]['TValue'] };
Review Comment:
Is this needed because of a Typescript version bump? This used to work
because the mapping [is part
of](https://github.com/apache/arrow-js/blob/main/src/row/struct.ts#L28-L30) the
`Struct['TValue']` aka `StructRowProxy`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
Divyanshu-s13 commented on PR #340: URL: https://github.com/apache/arrow-js/pull/340#issuecomment-3574209044 > Could you answer [#340 (comment)](https://github.com/apache/arrow-js/pull/340#issuecomment-3558090176) before we merge this? I have answered can you merge this. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
Divyanshu-s13 commented on PR #340: URL: https://github.com/apache/arrow-js/pull/340#issuecomment-3574205415 > Just to check. Originally, this pull request was just fixing a type but now it's also changing logic. Was the original patch not enough (as you noticed after writing a test case)? No. Writing test cases revealed that: Changing the type definition broke Vector access patterns The solution required both type changes AND logic changes (method overrides in StructBuilder) The logic changes were minimal (method signatures) because the underlying setValue() already handled plain objects at runtime. So Yes - Logic Changes Were Necessary Because: ✅ Type-only approach broke existing code (destructuring, .toJSON() calls) ✅ Tests revealed the issue - without tests, we wouldn't have caught this ✅ Solution required overriding methods - not just changing type signatures ✅ Runtime behavior needed adjustment - the setValue() already supported plain objects at runtime, but TypeScript didn't know that without the method overrides. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
kou commented on PR #340: URL: https://github.com/apache/arrow-js/pull/340#issuecomment-3573847416 Could you answer https://github.com/apache/arrow-js/pull/340#issuecomment-3558090176 before we merge this? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
Divyanshu-s13 commented on PR #340: URL: https://github.com/apache/arrow-js/pull/340#issuecomment-3573800792 @kou I have fixed the issue can you merge this. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
kou commented on code in PR #340: URL: https://github.com/apache/arrow-js/pull/340#discussion_r2552284423 ## yarn.lock: ## @@ -6649,4 +6649,4 @@ yocto-queue@^0.1.0: [email protected]: version "0.1.5" resolved "https://registry.yarnpkg.com/zstd-codec/-/zstd-codec-0.1.5.tgz#c180193e4603ef74ddf704bcc835397d30a60e42"; - integrity sha512-v3fyjpK8S/dpY/X5WxqTK3IoCnp/ZOLxn144GZVlNUjtwAchzrVo03h+oMATFhCIiJ5KTr4V3vDQQYz4RU684g== + integrity sha512-v3fyjpK8S/dpY/X5WxqTK3IoCnp/ZOLxn144GZVlNUjtwAchzrVo03h+oMATFhCIiJ5KTr4V3vDQQYz4RU684g== Review Comment: Could you revert this change? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
domoritz commented on PR #340: URL: https://github.com/apache/arrow-js/pull/340#issuecomment-3558090176 Just to check. Originally, this pull request was just fixing a type but now it's also changing logic. Was the original patch not enough (as you noticed after writing a test case)? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
Divyanshu-s13 commented on PR #340: URL: https://github.com/apache/arrow-js/pull/340#issuecomment-3557413009 > There was a request to add a test but it hasn't been added, right? You also haven't pushed the revert for the file changes, right? I have added a test case and pushed the revert the file change. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
raulcd commented on PR #340: URL: https://github.com/apache/arrow-js/pull/340#issuecomment-3557089798 There was a request to add a test but it hasn't been added, right? You also haven't pushed the revert for the file changes, right? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
Divyanshu-s13 commented on PR #340: URL: https://github.com/apache/arrow-js/pull/340#issuecomment-3557046392 > Could you check CI failures? It has been fixed now. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
Divyanshu-s13 commented on PR #340: URL: https://github.com/apache/arrow-js/pull/340#issuecomment-3557030413 > Thanks for the PR! I think you can revert the changes on `yarn.lock` and `eslint.config.js` not sure why we are removing the empty line there. Thanks for the review! Regarding the changes in yarn.lock and eslint.config.js, there was no intentional modification from my side. Those changes happened automatically during the local setup. I've now reverted both files back to their original state so that no unnecessary diffs appear and CI runs without issues.Now issue #90 has been fixed and all CI has been passed. Let me know if anything else needs to be updated! -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
Divyanshu-s13 commented on PR #340: URL: https://github.com/apache/arrow-js/pull/340#issuecomment-3556849307 > Could you check CI failures? @kou I’ve addressed the TypeScript error that was causing the CLI build failure. The issue is now fixed — could you please review the updated changes? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
kou commented on code in PR #340: URL: https://github.com/apache/arrow-js/pull/340#discussion_r2540683832 ## yarn.lock: ## Review Comment: Do we need this change? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] feat: Add missing type to `Struct` [arrow-js]
kou commented on PR #340: URL: https://github.com/apache/arrow-js/pull/340#issuecomment-3550981351 Could you check CI failures? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
