[BEAM-2442] BeamSql api surface test#3357
Conversation
|
@xumingmin @lukecwik can you take a look at this one? This might need to be merged before the other BeamSql PRs, otherwise it might cause a lot of conflict. |
mingmxu
left a comment
mingmxu
left a comment
There was a problem hiding this comment.
Thanks @xumingming .
Let BeamSql and BeamSqlCli extending BeamSqlEnv may not a good option regarding to BEAM-2446. You may change the the scope of SchemaPlus/BeamQueryPlanner to protected to avoid exposure.
|
@xumingmin , you are right, Let |
|
Please add a test like SdkCoreApiSurfaceTest which programmatically verifies that the API surface is clean. This test will catch breakages in the API surface in the future and not require manual review. |
|
There is a |
|
Sorry about that, did a quick pass and I missed it. |
takidau
left a comment
takidau
left a comment
There was a problem hiding this comment.
I'd like to have the CalciteUtils method names be a little more indicative of the equivalencies the pairs of maps/methods represent. Added comments as such. Otherwise LGTM. Let me know when those changes are made and I'll be happy to merge.
| */ | ||
| public class CalciteUtils { | ||
| private static final Map<Integer, SqlTypeName> SQL_TYPE_MAPPING = new HashMap<>(); | ||
| private static final Map<SqlTypeName, Integer> INVERSED_SQL_TYPE_MAPPING = new HashMap<>(); |
There was a problem hiding this comment.
I think these names would be more clear if they were named something like JAVA_TO_CALCITE_MAPPING and CALCITE_TO_JAVA_MAPPING.
| /** | ||
| * Get the corresponding {@code SqlTypeName} for an integer sql type. | ||
| */ | ||
| public static SqlTypeName getSqlTypeName(int type) { |
There was a problem hiding this comment.
They're both SQL types, though. Maybe call this toCalciteType() and the one below toJavaType?
| /** | ||
| * Generate {@code BeamSqlRecordType} from {@code RelDataType} which is used to create table. | ||
| */ | ||
| public static BeamSqlRecordType buildRecordType(RelDataType tableInfo) { |
There was a problem hiding this comment.
toBeamRecordType and toCalciteRecordType for the one below? It's not immediately clear these two methods are inverses of one another otherwise.
xumingming
force-pushed
the
BEAM-2442-api-surface-test
branch
from
976bad2 to
c0c4fd5
Compare
|
Update:
|
|
Thanks, merging. |
|
Merged, feel free to close. Thanks! |
5 participants
Summary:
org.apache.beam.dsls.sql.schemaCalciteUtils(which is not part of surface api) to avoid exposure.BeamSqlTableinterface which abstracts the beamtableconcept.RelDataType,RelProtoDataTypeare all removed from surface api,BeamSqlRecordTypeis the only class which represents the schema of a table.SqlTypeName.