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

add more debug info for TypeInfoConverter #426

Merged
merged 5 commits into from
Mar 2, 2023

Conversation

beyond-up
Copy link
Contributor

@beyond-up beyond-up commented Feb 28, 2023

#417

Signed-off-by:

Pre-Checklist

Note: Please complete ALL items in the following checklist.

  • I have read through the CONTRIBUTING.md documentation.
  • My code has the necessary comments and documentation (if needed).
  • I have added relevant tests.

Purpose

add more debug info and add check null for TypeInfoConverter

Related Issues

(#417)

New Behavior (screenshots if needed)

N/A

@hk-lrzy
Copy link
Collaborator

hk-lrzy commented Feb 28, 2023

@beyond-up Can we add more check after the code both in the NativeFlinkTypeInfoUtil and ColumnFlinkTypeInfoUtil

  public static RowTypeInfo getRowTypeInformation(List<ColumnInfo> columnInfos,
                                                  TypeInfoConverter typeInfoConverter) {

    String[] fieldNames = new String[columnInfos.size()];
    TypeInformation<?>[] fieldTypes = new TypeInformation[columnInfos.size()];

    for (int index = 0; index < columnInfos.size(); index++) {
      String type = StringUtils.lowerCase(columnInfos.get(index).getType());
      String name = columnInfos.get(index).getName();

      TypeInfo<?> typeInfo = typeInfoConverter.fromTypeString(type);

      //in here
      fieldNames[index] = name;
      fieldTypes[index] = toNativeFlinkTypeInformation(typeInfo);
    }

    return new RowTypeInfo(fieldTypes, fieldNames);
  }

Not every new type converter has enough validation rules, so add validation check after the typeInfoConverter invoke maybe more make sense? What do you think.

@hk-lrzy
Copy link
Collaborator

hk-lrzy commented Feb 28, 2023

default:
throw new IllegalArgumentException(String.format("Non type match for the type: %s.", engineType));

maybe follow will be better because we will know the field name directly.

throw new BitSailException(String.format("Field {} has no type match for the type: %s.", fieldname, engineType));

@beyond-up
Copy link
Contributor Author

ok, thanks

@hk-lrzy
Copy link
Collaborator

hk-lrzy commented Mar 2, 2023

@beyond-up Good job, i will merge it after CICD.

@beyond-up
Copy link
Contributor Author

@beyond-up Good job, i will merge it after CICD.

@hk-lrzy Thank you for your guidance and patience !

@hk-lrzy hk-lrzy merged commit 2ac540c into bytedance:master Mar 2, 2023
@beyond-up beyond-up deleted the dev_improve_typeInfoUtil branch March 2, 2023 09:10
hk-lrzy pushed a commit that referenced this pull request May 24, 2023
- Add more validation check for type info converter to avoid NPE.
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