close
Skip to content

[Feature][Spark] Support multiple tables read and write#7283

Merged
Hisoka-X merged 16 commits into
apache:devfrom
Carl-Zhou-CN:spark-rewrite
Aug 14, 2024
Merged

[Feature][Spark] Support multiple tables read and write#7283
Hisoka-X merged 16 commits into
apache:devfrom
Carl-Zhou-CN:spark-rewrite

Conversation

@Carl-Zhou-CN

Copy link
Copy Markdown
Member

close #7009

Purpose of this pull request

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

@github-actions github-actions Bot added core SeaTunnel core module Spark format labels Jul 29, 2024
@github-actions github-actions Bot removed the format label Jul 29, 2024
@github-actions github-actions Bot added the e2e label Jul 29, 2024
@hailin0

hailin0 commented Aug 1, 2024

Copy link
Copy Markdown
Member

cc @CheneyYin

this.jobName = config.getString("job.name");
}
sparkConf = createSparkConf();
sparkConf.set("spark.sql.unsafe.enabled", "false");

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.

why disable unsafe?

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.

I'll remove it. Thank you

Comment on lines 83 to 85
public InternalRow convert(SeaTunnelRow seaTunnelRow) throws IOException {
validate(seaTunnelRow);
return (InternalRow) convert(seaTunnelRow, dataType);
return parcel(seaTunnelRow, (SeaTunnelRowType) dataType);
}

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.

It might be more reliable to add unit tests for it. It would be best if it covers all data types.

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.

I will refactor and consolidate this part of the logic and provide test cases.

Comment on lines 267 to 269
public SeaTunnelRow reconvert(InternalRow engineRow) throws IOException {
return (SeaTunnelRow) reconvert(engineRow, dataType);
return unpack(engineRow, (SeaTunnelRowType) dataType);
}

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.

ditto

return newRow;
}

public GenericRowWithSchema parcel(SeaTunnelRow seaTunnelRow) {

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.

ditto

return (SeaTunnelRow) reconvert(engineRow, dataType);
}

public SeaTunnelRow unpack(GenericRowWithSchema engineRow) throws IOException {

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.

ditto

List<String> fieldNames = new ArrayList<>();
List<SeaTunnelDataType<?>> fieldTypes = new ArrayList<>();
List<ColumnWithIndex> columnWithIndexes = new ArrayList<>();
fieldTypes.sort(Comparator.comparingInt(o -> o.getSqlType().ordinal()));

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.

fieldTypes is empty. It's meaningless to sort it.

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.

It's not being used. I'll remove it. Thank you


public class SchemaUtil {

public static ColumnWithIndex[] mergeSchema(List<CatalogTable> catalogTables) {

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.

It would be better to add unit test cases.

@hailin0

hailin0 commented Aug 10, 2024

Copy link
Copy Markdown
Member

waiting for this pr merge
#7356

@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.

Please add a screenshot of the log for multi-table mapping

Comment on lines +107 to +111
// if (catalogTables.size() != 1) {
// throw new SeaTunnelException(
// String.format("Unsupported table number: %d on flink",
// catalogTables.size()));
// }

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.

why?

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.

This is specifically for multi-table exceptions and can be removed when both spark and flink are implemented

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.

flink is currently not supported

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.

It affects not only flink but also 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.

So flink will not throw exception now?

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.

yes, I think flink is about to implement multiple tables as well

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.

Ok. Please clean the code.

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.

Let's clean it up when flink implements multiple tables

@Carl-Zhou-CN

Copy link
Copy Markdown
Member Author

image

}
this.sparkSession = builder.getOrCreate();
createStreamingContext();

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.

FYA

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.

done, good fix

TableTransformFactoryContext context =
new TableTransformFactoryContext(
Collections.singletonList(dataset.getCatalogTable()),
Collections.singletonList(dataset.getCatalogTables().get(0)),

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.

Maybe you can use dataset.getCatalogTables() instead of Collections.singletonList(dataset.getCatalogTables().get(0)) ?. Like this:

TableTransformFactoryContext context =                        
    new TableTransformFactoryContext(
        dataset.getCatalogTables(),
        ReadonlyConfig.fromConfig(pluginConfig),
        classLoader);

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.

done, excellent advice

Dataset<Row> stream = tableInfo.getDataset();
SeaTunnelDataType<?> inputDataType = tableInfo.getCatalogTable().getSeaTunnelRowType();
SeaTunnelDataType<?> inputDataType =
tableInfo.getCatalogTables().get(0).getSeaTunnelRowType();

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.

Does Covenant transform only deal with the first one?

@Carl-Zhou-CN Carl-Zhou-CN Aug 11, 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.

This is because 'transform' now supports only single tables

@Carl-Zhou-CN Carl-Zhou-CN requested a review from CheneyYin August 11, 2024 04:59

@CheneyYin CheneyYin left a comment

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.

LGTM

@Carl-Zhou-CN Carl-Zhou-CN requested a review from hailin0 August 12, 2024 01:57
mockServerClient.retrieveRecordedRequests(
request().withPath("/example/httpMultiTableContentSink").withMethod("POST"),
Format.JSON);
mockServerClient.clear(

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.

Sorry, I forgot to clear the records, which caused the run to fail. This test case will run multiple times. Please help add it

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.

Thank you very much @jackyyyyyssss ,Please cc

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.

LGTM

@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.

LGTM

cc @Hisoka-X

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved core SeaTunnel core module e2e Spark

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature][Spark] Support multiple tables read and write

5 participants