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.

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

[Improve][transforms-v2][SQL] Support can use column.a, column['a'], and column[0] to get the data #5402

Closed
wants to merge 2 commits into from

Conversation

Zhouwen-CN
Copy link
Contributor

#5245

Purpose of this pull request

Check list

@EricJoy2048
Copy link
Member

Good features, Please wait CI complete.

EricJoy2048
EricJoy2048 previously approved these changes Sep 11, 2023
Copy link
Member

@EricJoy2048 EricJoy2048 left a comment

Choose a reason for hiding this comment

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

LGTM

@EricJoy2048
Copy link
Member

@rewerma PATL

@Zhouwen-CN
Copy link
Contributor Author

What is this mistake? Did I cause it

@EricJoy2048
Copy link
Member

What is this mistake? Did I cause it

No, it's not you problem. Can you rebase the dev branch and push again ?

@Zhouwen-CN
Copy link
Contributor Author

What is this mistake? Did I cause it

No, it's not you problem. Can you rebase the dev branch and push again ?

Ok, tonight

@Zhouwen-CN
Copy link
Contributor Author

What is this mistake? Did I cause it

No, it's not you problem. Can you rebase the dev branch and push again ?

Is this how it is done?

@EricJoy2048
Copy link
Member

What is this mistake? Did I cause it

No, it's not you problem. Can you rebase the dev branch and push again ?

Is this how it is done?

It seems some thing is wrong. Some modifications that should not be submitted in this PR.

@EricJoy2048
Copy link
Member

EricJoy2048 commented Sep 11, 2023

How to rebase?

git checkout chen0623-bak:sql
git fetch ${the apache remote}
git rebase ${the apache remote}/dev

@Zhouwen-CN
Copy link
Contributor Author

Zhouwen-CN commented Sep 11, 2023

sync fork
image
git fetch origin (at local)
git checkout sql (at local)
git rebase dev (at local)
git push sql (Prompt me whether to merge or rebase, I chose rebase)

like this
image

@EricJoy2048
Copy link
Member

sync fork image git fetch origin (at local) git checkout sql (at local) git rebase dev (at local) git push sql (Prompt me whether to merge or rebase, I chose rebase)

like this image

For you current state, I think you can do it like this:

git fetch --all
git checkout dev
git pull
git checkout sql
git reset --hard bb1371a04599b0f2c421dc97504e6bb43b3ef8f8
git rebase dev (in this step, you need fix the conflict, and use git rebase --continue at the appropriate time)
git push -f

@Zhouwen-CN
Copy link
Contributor Author

@EricJoy2048
Is this how it is done?
Thank, I get it

@EricJoy2048
Copy link
Member

@wu-a-ge PTAL. I have found that you have extensive experience in this field. Can you help review this PR?

@EricJoy2048
Copy link
Member

@rewerma PTAL

@Zhouwen-CN
Copy link
Contributor Author

Zhouwen-CN commented Sep 13, 2023

@EricJoy2048
I git it, I'm back from Internet

transform-v2-it-part-2 (8, ubuntu-latest) :
	when map size lt 5
	scala use Map$Map1 Map$Map2 Map$Map3 Map$Map4
	when map size gte 5
	scala use Map$HashTrieMap
	So we can avoid this error by using Map
		Modify SeaTunnelRowConverter 207 line
		Modify SeaTunnelRowConverter 173 line
        Or modify config file map size gte 5

FakeSource does not specify the rows parameter for testing without reporting errors because map.size default=5

I changed the map length of the test file to 5
The local test has passed

Copy link
Contributor

Choose a reason for hiding this comment

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

The map and array values in the test case are only strings, can we consider other types? For example,null,int,date,timestamp

Copy link
Contributor Author

@Zhouwen-CN Zhouwen-CN Sep 16, 2023

Choose a reason for hiding this comment

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

Are you saying that the key of the map cannot be certain to be a string
This is indeed a problem
I think Visitor should be used to access it

Because it is JSON serialization, the key will be parsed as a string, even if it is an int
The type is taken from upstream, I think there should be no problem

Copy link
Contributor

Choose a reason for hiding this comment

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

What I am talking about is the value, and it is necessary to consider that after these values are taken out, the downstream sink can write correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The generics of array can only be basic types
If the value type of a map is date, time, or timestamp, the underlying type is actually a Java8 time type
Jackson will report an error during serialization and needs to register the JavaTimeModule module
Specific dependencies need to be added as well

@Zhouwen-CN
Copy link
Contributor Author

Zhouwen-CN commented Sep 16, 2023

I added a Visitor to obtain the key or index
The key of a map may be int, double, or string, it may not necessarily be a string
Although it will be converted to a string during JSON serialization, But we may have other serializers
I don't know if this is suitable or not
and When ArrayIndexOutOfBoundsException return null, Ensure the robustness of the program

I think the parameter Object[] inputFields should also be passed in when parsing types
Because if I don't pass in parameters, I need to make a lot of non null judgments
And it cannot parse the subscript calculation of column[c1+1] as a reference column

@EricJoy2048
Copy link
Member

Hi, Way are you close this pr?

@Zhouwen-CN
Copy link
Contributor Author

Hi, Way are you close this pr?

I think the visitor mode extension is better
I will improve it in the new PR

Referring to select *, its implementation is simpler and more decoupled

@sxiongzhang
Copy link

I will improve it in the new PR

hello ,has the pr been submitted?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants