Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Feature] Support multiple fields in sequence-group #3461

Merged
merged 3 commits into from
Jun 12, 2024
Merged

[Feature] Support multiple fields in sequence-group #3461

merged 3 commits into from
Jun 12, 2024

Conversation

xiangyuf
Copy link
Contributor

@xiangyuf xiangyuf commented Jun 2, 2024 •
edited
Loading

Purpose

Linked issue: close #3372

Use UserDefinedSeqComparator to support multiple fields in sequence-group

Tests

Added UT:

  • PartialUpdateMergeFunctionTest.testMultiSequenceFields
  • PartialUpdateMergeFunctionTest.testMultiSequenceFieldsDefaultAggFunc
  • PartialUpdateMergeFunctionTest.testMultiSequenceFieldsDefinedNoField
  • PartialUpdateMergeFunctionTest.testMultiSequenceFieldsRepeatDefine
  • PartialUpdateMergeFunctionTest.testMultiSequenceFieldsAdjustProjectionRepeatProject
  • PartialUpdateMergeFunctionTest.testMultiSequenceFieldsAdjustProjectionProject
  • PartialUpdateMergeFunctionTest.testMultiSequenceFieldsAdjustProjectionAllFieldsProject
  • PartialUpdateMergeFunctionTest.testMultiSequenceFieldsFirstValue
  • PartialUpdateMergeFunctionTest.testMultiSequenceFieldsPartialUpdateWithAggregation
  • PartialUpdateMergeFunctionTest.testMultiSequenceFieldsPartialUpdateWithAggregationProjectPushDown

Added IT:

  • PartialUpdateITCase.testMultiFieldsSequenceGroup
  • PartialUpdateITCase.testMultiFieldsSequencePartialUpdateWithAggregation

API and Format

No

Documentation

@xiangyuf xiangyuf changed the title [WIP][Feature] Support multiple fields in sequence-group [Feature] Support multiple fields in sequence-group Jun 3, 2024
@xiangyuf xiangyuf marked this pull request as ready for review June 3, 2024 17:33
@xiangyuf xiangyuf force-pushed the multi_fields branch 7 times, most recently from 1a21441 to 4622f12 Compare June 4, 2024 14:43
@xiangyuf xiangyuf marked this pull request as draft June 4, 2024 14:59
@xiangyuf xiangyuf marked this pull request as ready for review June 4, 2024 14:59
@JingsongLi
Copy link
Contributor

@xiangyuf Can you run tests in your local? I feel quite tired from clicking on the testing button...

@xiangyuf
Copy link
Contributor Author

xiangyuf commented Jun 5, 2024

@xiangyuf Can you run tests in your local? I feel quite tired from clicking on the testing button...

It works fine now both on my local and my repository.
image

@JingsongLi
Copy link
Contributor

@xiangyuf thanks!

@xiangyuf xiangyuf requested a review from JingsongLi June 5, 2024 12:19
@FangYongs
Copy link
Contributor

Thanks @xiangyuf , left some minor comments

@xiangyuf xiangyuf requested a review from FangYongs June 7, 2024 12:15
@xiangyuf
Copy link
Contributor Author

image
@JingsongLi The CI workflow looks good in my repository.

JingsongLi
JingsongLi approved these changes Jun 12, 2024
View reviewed changes
Copy link
Contributor

@JingsongLi JingsongLi left a comment

Choose a reason for hiding this comment

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

+1

@JingsongLi JingsongLi merged commit 1b5f090 into apache:master Jun 12, 2024
@xiangyuf xiangyuf mentioned this pull request Jul 12, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JingsongLi JingsongLi JingsongLi approved these changes

@FangYongs FangYongs Awaiting requested review from FangYongs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Support multiple fields in sequence-group
3 participants
@xiangyuf @JingsongLi @FangYongs