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

[Feature][Catalog] Add InMemoryCatalog for test and add new getCatalogTableFromConfig method #5485

Merged
merged 5 commits into from
Sep 19, 2023

Conversation

Hisoka-X
Copy link
Member

Purpose of this pull request

  1. Add new getCatalogTableFromConfig method, so that we can directly get CatalogTable from source config. It will be used when refactor Catalog for move create CatalogTable logic to TableSourceFactory instance.
  2. To prove new method can work fine, add test suite InMemoryCatalog, after this we can test catalog without any third party database.

Check list

@Hisoka-X
Copy link
Member Author

cc @ruanwenjun @hailin0

@@ -61,12 +56,7 @@ public void testSimpleSchemaParse() throws FileNotFoundException, URISyntaxExcep
@Test
public void testComplexSchemaParse() throws FileNotFoundException, URISyntaxException {
String path = getTestConfigFile("/conf/complex.schema.conf");
Config config =
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

@@ -56,11 +54,4 @@ public interface CatalogOptions {
.withDescription(
"The table names RegEx of the database to capture."
+ "The table name needs to include the database name, for example: database_.*\\.table_.*");

OptionRule.Builder BASE_RULE =
Copy link
Member Author

Choose a reason for hiding this comment

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

I also removed some useless code.

* </a>
*/
@Deprecated
public static List<CatalogTable> getCatalogTablesFromConfig(
Copy link
Member Author

Choose a reason for hiding this comment

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

after we delete getCatalogTables method, I will change this method name to getCatalogTables

ruanwenjun
ruanwenjun previously approved these changes Sep 15, 2023
Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Carl-Zhou-CN Carl-Zhou-CN left a comment

Choose a reason for hiding this comment

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

+1

@liugddx
Copy link
Member

liugddx commented Sep 18, 2023

Is this not triggering CI?

@Hisoka-X
Copy link
Member Author

Is this not triggering CI?

Triggered. But not async with main repo. Let me investigate it.
image

Copy link
Member

@liugddx liugddx left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

+1

@hailin0 hailin0 merged commit cae66b6 into apache:dev Sep 19, 2023
5 checks passed
@Hisoka-X
Copy link
Member Author

@Hisoka-X Hisoka-X deleted the catalog_refactor branch September 19, 2023 08:45
gnehil pushed a commit to gnehil/seatunnel that referenced this pull request Oct 12, 2023
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.

5 participants