GH-43118: [JS] Add interval for unit MONTH_DAY_NANO #43117

handstuyennn · 2024-07-02T11:40:38Z

Rationale for this change

This PR handle for Interval type with MONTH_DAY_NANO unit, This is currently not supported in Arrow JS implementation.
See #43118

What changes are included in this PR?

Handle Interval with MONTH_DAY_NAO unit with relations
Adding tests for builder and unittest

Are these changes tested?

Yes. Unit tests have been added

GitHub Issue: [JS]: Handle Interval with MONTH_DAY_NANO unit #43118

github-actions · 2024-07-02T11:41:01Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2024-07-02T11:49:28Z

⚠️ GitHub issue #43118 has been automatically assigned in GitHub to PR creator.

domoritz

Looks good overall. Please fix the CI issues.

I'll defer to @trxcllnt for a final ok.

kou · 2024-07-02T23:24:40Z

js/test/unit/vector/interval-month-day-nano-tests.ts

Could you add our license header?

I added the license

kou · 2024-07-02T23:25:59Z

Could you fix lint failures?

https://github.com/apache/arrow/actions/runs/9760275633/job/26955306166?pr=43117#step:5:150

$ eslint src test

/build/js/src/type.ts
  492:132  error  Missing semicolon  @typescript-eslint/semi

/build/js/src/util/buffer.ts
  134:18  warning  Move generator function 'wrap' to the outer scope        unicorn/consistent-function-scoping
  172:18  warning  Move async generator function 'wrap' to the outer scope  unicorn/consistent-function-scoping

/build/js/test/generate-test-data.ts
  327:5   warning  Unnecessarily cloning an array  unicorn/no-useless-spread
  349:5   warning  Unnecessarily cloning an array  unicorn/no-useless-spread
  370:20  warning  Unnecessarily cloning an array  unicorn/no-useless-spread
  380:20  warning  Unnecessarily cloning an array  unicorn/no-useless-spread
  488:51  warning  Unnecessarily cloning an array  unicorn/no-useless-spread
  626:49  warning  Unnecessarily cloning an array  unicorn/no-useless-spread

/build/js/test/unit/vector/interval-month-day-nano-tests.ts
   1:56  error  Strings must use singlequote                                 @typescript-eslint/quotes
   6:25  error  Expected a semicolon                                         @typescript-eslint/member-delimiter-style
  16:5   error  This `if` statement can be replaced by a ternary expression  unicorn/prefer-ternary
  28:42  error  Missing semicolon                                            @typescript-eslint/semi
  39:5   error  Use a `for-of` loop instead of this `for` loop               unicorn/no-for-loop
  41:9   error  Use a `for-of` loop instead of this `for` loop               unicorn/no-for-loop
  84:4   error  Newline required at end of file but not found                eol-last

✖ 16 problems (8 errors, 8 warnings)
  8 errors and 0 warnings potentially fixable with the `--fix` option.

handstuyennn · 2024-07-03T02:13:28Z

Could you fix lint failures?

https://github.com/apache/arrow/actions/runs/9760275633/job/26955306166?pr=43117#step:5:150

$ eslint src test

/build/js/src/type.ts
  492:132  error  Missing semicolon  @typescript-eslint/semi

/build/js/src/util/buffer.ts
  134:18  warning  Move generator function 'wrap' to the outer scope        unicorn/consistent-function-scoping
  172:18  warning  Move async generator function 'wrap' to the outer scope  unicorn/consistent-function-scoping

/build/js/test/generate-test-data.ts
  327:5   warning  Unnecessarily cloning an array  unicorn/no-useless-spread
  349:5   warning  Unnecessarily cloning an array  unicorn/no-useless-spread
  370:20  warning  Unnecessarily cloning an array  unicorn/no-useless-spread
  380:20  warning  Unnecessarily cloning an array  unicorn/no-useless-spread
  488:51  warning  Unnecessarily cloning an array  unicorn/no-useless-spread
  626:49  warning  Unnecessarily cloning an array  unicorn/no-useless-spread

/build/js/test/unit/vector/interval-month-day-nano-tests.ts
   1:56  error  Strings must use singlequote                                 @typescript-eslint/quotes
   6:25  error  Expected a semicolon                                         @typescript-eslint/member-delimiter-style
  16:5   error  This `if` statement can be replaced by a ternary expression  unicorn/prefer-ternary
  28:42  error  Missing semicolon                                            @typescript-eslint/semi
  39:5   error  Use a `for-of` loop instead of this `for` loop               unicorn/no-for-loop
  41:9   error  Use a `for-of` loop instead of this `for` loop               unicorn/no-for-loop
  84:4   error  Newline required at end of file but not found                eol-last

✖ 16 problems (8 errors, 8 warnings)
  8 errors and 0 warnings potentially fixable with the `--fix` option.

I updated it

trxcllnt · 2024-07-03T21:29:14Z

@handstuyennn Thanks for the PR. Would you mind also removing these two lines so the new functionality is validated against the other Arrow implementations?

handstuyennn · 2024-07-04T02:23:44Z

@handstuyennn Thanks for the PR. Would you mind also removing these two lines so the new functionality is validated against the other Arrow implementations?

I updated this

domoritz · 2024-07-05T12:55:19Z

Can you fix the integration test?

domoritz · 2024-07-25T12:03:11Z

@handstuyennn could you look into the last remaining test so we can merge this?

handstuyennn · 2024-07-26T05:06:52Z

@handstuyennn could you look into the last remaining test so we can merge this?

I will handle it this weekend

domoritz · 2024-09-05T12:24:27Z

dev/archery/archery/integration/datagen.py


-        generate_month_day_nano_interval_case(),
+        generate_month_day_nano_interval_case()
+        .skip_tester('JS'),


Why skip this test? Isn't this the point of this pull request?

Hi @domoritz The issues about interval data in JSON format just been fixed in the latest commit. Please check it. Thank you

js/src/visitor/vectorloader.ts

trxcllnt

Need to revisit changes to vectorloader.ts.

trxcllnt · 2024-09-30T22:09:51Z

js/src/visitor/get.ts

+    (data.type.unit === IntervalUnit.MONTH_DAY_NANO)
+        ? getIntervalMonthDayNano(data as Data<IntervalMonthDayNano>, index)
+        : (data.type.unit === IntervalUnit.DAY_TIME)
+            ? getIntervalDayTime(data as Data<IntervalDayTime>, index)
+            : getIntervalYearMonth(data as Data<IntervalYearMonth>, index);


Could we split these conditions out into separate methods and assign them to the GetVisitor prototype separately?

trxcllnt · 2024-09-30T22:10:13Z

js/src/visitor/set.ts

+    (data.type.unit === IntervalUnit.MONTH_DAY_NANO)
+        ? setIntervalMonthDaynano(data as Data<IntervalMonthDayNano>, index, value)
+        : (data.type.unit === IntervalUnit.DAY_TIME)
+            ? setIntervalDayTime(data as Data<IntervalDayTime>, index, value)
+            : setIntervalYearMonth(data as Data<IntervalYearMonth>, index, value);


Same question here.

handstuyennn requested review from domoritz and trxcllnt as code owners July 2, 2024 11:40

github-actions bot added Component: JavaScript awaiting review Awaiting review labels Jul 2, 2024

handstuyennn changed the title ~~MINOR [JS]: Add interval for unit MONTH_DAY_NANO~~ GH-43118: [JS] Add interval for unit MONTH_DAY_NANO Jul 2, 2024

domoritz approved these changes Jul 2, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Jul 2, 2024

kou reviewed Jul 2, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Jul 2, 2024

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 3, 2024

handstuyennn requested review from assignUser and raulcd as code owners July 4, 2024 02:21

handstuyennn force-pushed the js-interval-month-day-nano branch from 89ae4a3 to 01a0288 Compare July 4, 2024 02:21

handstuyennn force-pushed the js-interval-month-day-nano branch from 01a0288 to ded7890 Compare September 4, 2024 02:04

handstuyennn requested a review from jonkeane as a code owner September 4, 2024 02:04

handstuyennn added 3 commits September 5, 2024 07:05

[JS]: Add interval for unit MONTH_DAY_NANO

c2d5f55

[CI][JS] Update code formatting

9f2c015

[CI][JS] Update semicolon

be61e7e

handstuyennn added 4 commits September 5, 2024 07:05

[JS]: Remove skip js test and update ci test

09337f2

[JS]: Update integration tests for interval

159c4fc

[JS]: Format Python code

1b4d1be

[JS]: Skip js integration tests for interval

ade6c83

handstuyennn force-pushed the js-interval-month-day-nano branch from 5bd3763 to ade6c83 Compare September 5, 2024 00:05

domoritz reviewed Sep 5, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 5, 2024

hangxingliu added 2 commits September 27, 2024 12:10

[JS] Add utility functions for encoding interval data from JSON IPC data

88ffddb

[JS] Restore js integration tests for interval

c51a503

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 27, 2024

trxcllnt reviewed Sep 30, 2024

View reviewed changes

js/src/visitor/vectorloader.ts Show resolved Hide resolved

trxcllnt requested changes Sep 30, 2024

View reviewed changes

trxcllnt self-requested a review September 30, 2024 22:08

trxcllnt reviewed Sep 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-43118: [JS] Add interval for unit MONTH_DAY_NANO #43117

GH-43118: [JS] Add interval for unit MONTH_DAY_NANO #43117

handstuyennn commented Jul 2, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Jul 2, 2024

github-actions bot commented Jul 2, 2024

domoritz left a comment

kou Jul 2, 2024

handstuyennn Jul 3, 2024

kou commented Jul 2, 2024

handstuyennn commented Jul 3, 2024

trxcllnt commented Jul 3, 2024 •

edited

Loading

handstuyennn commented Jul 4, 2024

domoritz commented Jul 5, 2024

domoritz commented Jul 25, 2024

handstuyennn commented Jul 26, 2024

domoritz Sep 5, 2024

hangxingliu Sep 27, 2024

trxcllnt left a comment

trxcllnt Sep 30, 2024

trxcllnt Sep 30, 2024

GH-43118: [JS] Add interval for unit MONTH_DAY_NANO #43117

Are you sure you want to change the base?

GH-43118: [JS] Add interval for unit MONTH_DAY_NANO #43117

Conversation

handstuyennn commented Jul 2, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

github-actions bot commented Jul 2, 2024

github-actions bot commented Jul 2, 2024

domoritz left a comment

Choose a reason for hiding this comment

kou Jul 2, 2024

Choose a reason for hiding this comment

handstuyennn Jul 3, 2024

Choose a reason for hiding this comment

kou commented Jul 2, 2024

handstuyennn commented Jul 3, 2024

trxcllnt commented Jul 3, 2024 • edited Loading

handstuyennn commented Jul 4, 2024

domoritz commented Jul 5, 2024

domoritz commented Jul 25, 2024

handstuyennn commented Jul 26, 2024

domoritz Sep 5, 2024

Choose a reason for hiding this comment

hangxingliu Sep 27, 2024

Choose a reason for hiding this comment

trxcllnt left a comment

Choose a reason for hiding this comment

trxcllnt Sep 30, 2024

Choose a reason for hiding this comment

trxcllnt Sep 30, 2024

Choose a reason for hiding this comment

handstuyennn commented Jul 2, 2024 •

edited by github-actions bot

Loading

trxcllnt commented Jul 3, 2024 •

edited

Loading