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

[Fix][Connector-V2][ES]source deserializer error and inappropriate #4233

Merged
merged 9 commits into from
Apr 1, 2023

Conversation

kpretty
Copy link
Contributor

@kpretty kpretty commented Feb 27, 2023

Purpose of this pull request

for timestamp type data this is wrong

时间解析源代码

The error message is as follows

时间戳转换失败

Therefore, for the timestamp type, it should be parsed into LocalDateTime like this
LocalDateTime.ofInstant(Instant.ofEpochMilli(Long.parseLong(fieldValue)), ZoneId.systemDefault());

string parse inappropriate

It is inappropriate to directly call toString() for string type data, and the sink will receive redundant "
处理 TextNode 不恰当

final

The current pr has already dealt with the above problems, and the final effect is as follows
正确处理

Check list

@kpretty
Copy link
Contributor Author

kpretty commented Feb 27, 2023

e2e for es seems to be problematic

@TyrantLucifer
Copy link
Member

e2e for es seems to be problematic

Yup, it seems that this change effect the origin test cases.

@TyrantLucifer TyrantLucifer added this to the 2.3.1 milestone Feb 27, 2023
@Hisoka-X
Copy link
Member

Hisoka-X commented Mar 4, 2023

Can you fix e2e for this? Thanks

@kpretty
Copy link
Contributor Author

kpretty commented Mar 4, 2023

@Hisoka-X ok, I'll fix it in the next pr

@Hisoka-X
Copy link
Member

Hisoka-X commented Mar 4, 2023

@Hisoka-X ok, I'll fix it in the next pr

ok for me, but we will waiting all ci pass to merge this PR.

@TyrantLucifer
Copy link
Member

TyrantLucifer commented Mar 4, 2023

@Hisoka-X ok, I'll fix it in the next pr

ok for me, but we will waiting all ci pass to merge this PR.

CI also has some problems:

image

@TyrantLucifer
Copy link
Member

@iture123 PTAL.

@kpretty
Copy link
Contributor Author

kpretty commented Mar 4, 2023

@Hisoka-X ok, I'll fix it in the next pr

ok for me, but we will waiting all ci pass to merge this PR.

CI also has some problems:

image

Yes, I'm testing locally

@kpretty
Copy link
Contributor Author

kpretty commented Mar 5, 2023

@Hisoka-X @TyrantLucifer TBR

Hisoka-X
Hisoka-X previously approved these changes Mar 5, 2023
Copy link
Member

@Hisoka-X Hisoka-X left a comment

Choose a reason for hiding this comment

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

LGTM

@TyrantLucifer
Copy link
Member

cc @iture123 PTAL

@kpretty
Copy link
Contributor Author

kpretty commented Mar 14, 2023

@TyrantLucifer @Hisoka-X This conflict is caused by the different ways of processing data types that es is not mapped to. The processing logic of the dev branch throws an exception, but I think this method is not very friendly. Is it better to give a default type user experience

@EricJoy2048
Copy link
Member

Please fix the conflicts

@kpretty
Copy link
Contributor Author

kpretty commented Mar 17, 2023

@EricJoy2048 fixed

@TyrantLucifer TyrantLucifer removed this from the 2.3.1 milestone Mar 19, 2023
@@ -150,7 +158,8 @@ Object convertValue(SeaTunnelDataType<?> fieldType, String fieldValue)
LocalDateTime localDateTime = parseDate(fieldValue);
return localDateTime.toLocalTime();
} else if (LocalTimeType.LOCAL_DATE_TIME_TYPE.equals(fieldType)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ES support Multiple date formats, and you should enhance it without removing the old implementation.ES date type can be defined as follows
{
"type": "date",
"format": "yyyy-MM-dd HH:mm:ss||yyyy-MM-dd||epoch_millis"
}

@kpretty kpretty closed this Mar 26, 2023
@kpretty kpretty deleted the fix-es-source-deserializer branch March 26, 2023 09:52
@kpretty kpretty restored the fix-es-source-deserializer branch March 26, 2023 10:04
@kpretty kpretty reopened this Mar 26, 2023
@kpretty
Copy link
Contributor Author

kpretty commented Mar 26, 2023

@iture123 there is indeed a problem, I have moved the logic of timestamp processing into parseDate

@TyrantLucifer TyrantLucifer merged commit 15530d2 into apache:dev Apr 1, 2023
MonsterChenzhuo pushed a commit to MonsterChenzhuo/incubator-seatunnel that referenced this pull request Apr 19, 2023
apache#4233)

* [Fix][Connector-V2][ES]source deserializer error and inappropriate

* [WIP][Connector-V2][ES]try fix e2e

* [WIP][Connector-V2][ES]try fix e2e

* [WIP][Connector-V2][ES]try fix e2e

* [WIP][Connector-V2][ES]try fix e2e

* [Fix][Connector-V2][ES]es special types may result in a null pointer. e.g. ip type

* [Fix][Connector-V2][ES]fix the parsing exception when the es time type is epoch_millis
ic4y pushed a commit to ic4y/incubator-seatunnel that referenced this pull request May 22, 2023
apache#4233)

* [Fix][Connector-V2][ES]source deserializer error and inappropriate

* [WIP][Connector-V2][ES]try fix e2e

* [WIP][Connector-V2][ES]try fix e2e

* [WIP][Connector-V2][ES]try fix e2e

* [WIP][Connector-V2][ES]try fix e2e

* [Fix][Connector-V2][ES]es special types may result in a null pointer. e.g. ip type

* [Fix][Connector-V2][ES]fix the parsing exception when the es time type is epoch_millis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] [Connector-V2][ES] timestamp type conversion fails and sink stage there will be redundant "
5 participants