close
Skip to content

[Improve][Transform] Sql transform support inner strucy query#6484

Merged
hailin0 merged 4 commits into
apache:devfrom
liunaijie:sql-improve
Mar 15, 2024
Merged

[Improve][Transform] Sql transform support inner strucy query#6484
hailin0 merged 4 commits into
apache:devfrom
liunaijie:sql-improve

Conversation

@liunaijie

@liunaijie liunaijie commented Mar 11, 2024

Copy link
Copy Markdown
Member

Purpose of this pull request

close #6468

Now the struct query will be recognized as table_name.column_name, I am not change this because i think there are someone still use this query pattern. just add a backfill logical to handle it.
For map stuct, it should be the latest struct.

Does this PR introduce any user-facing change?

No

How was this patch tested?

add e2e test

Check list

@liunaijie liunaijie force-pushed the sql-improve branch 4 times, most recently from 1838568 to 3e6f188 Compare March 11, 2024 06:25

@hailin0 hailin0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your contribution, need to update the documentation?

@hailin0

hailin0 commented Mar 11, 2024

Copy link
Copy Markdown
Member

LGTM

waiting for ci passed

@liunaijie

Copy link
Copy Markdown
Member Author

Thanks for your contribution, need to update the documentation?

Updated

Comment on lines +65 to +67
@DisabledOnContainer(
value = {},
type = {EngineType.SPARK})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @liunaijie . Why disabled on spark?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seem like the spark translation have some problem when use map type. You can create an issue and add it into your disabledReason.

Comment on lines +206 to +210
try {
String columnName = columnExp.getColumnName();
int idx = inputRowType.indexOf(columnName);
return inputFields[idx];
} catch (IllegalArgumentException e) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add a new indexOf method in SeaTunnelRowType which no IllegalArgumentException exception is thrown. Then use the result of the index to determine whether there is a corresponding fieldname. Because data processing is performed through try catch at now, the performance is particularly bad.

Comment on lines +106 to +109
try {
String columnName = columnExp.getColumnName();
return inputRowType.getFieldType(inputRowType.indexOf(columnName));
} catch (IllegalArgumentException e) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto

@Hisoka-X Hisoka-X left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM if ci passed

Comment on lines +222 to +228
int idx = ((SeaTunnelRowType) parDataType).indexOf(columnNames[i], false);
if (idx == -1) {
throw new IllegalArgumentException(
String.format("can't find field [%s]", fullyQualifiedName));
}
parDataType = ((SeaTunnelRowType) parDataType).getFieldType(idx);
res = parRowValues.getFields()[idx];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  public int indexOf(String fieldName, boolean throwExceptionWhenNotFound) {
        for (int i = 0; i < fieldNames.length; i++) {
            if (fieldNames[i].equals(fieldName)) {
                return i;
            }
        }
        if (throwExceptionWhenNotFound) {
            throw new IllegalArgumentException(String.format("can't find field [%s]", fieldName));
        } else {
            return -1;
        }
    }
``` Logical conflict?
cc  @hailin0 @Hisoka-X 

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@liunaijie liunaijie Mar 14, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

check the field name is in this struct. if not exist, choose throw exception or return -1 index let's the caller handle.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

image Repetitive code logic

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @zhilinli123 for ping me. I think it's different. indexOf only throw sub fieldname. The fullyQualifiedName and columnNames[i] not equals. Like field user.age, we should throw user.age not age.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i agree with @Hisoka-X

@Hisoka-X Hisoka-X requested a review from zhilinli123 March 14, 2024 14:07
@hailin0 hailin0 merged commit 77ffa56 into apache:dev Mar 15, 2024
@liunaijie liunaijie deleted the sql-improve branch March 15, 2024 05:26
chaorongzhi pushed a commit to chaorongzhi/seatunnel that referenced this pull request Aug 21, 2024
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.

[Improve][SQL-Transform] improve sql transform to support struct query like row_a.inner_row_b.column_c

4 participants