fix(csharp): normalize TypeName once in TryGetSchemaFromManifest#337
fix(csharp): normalize TypeName once in TryGetSchemaFromManifest#337eric-wang-1990 merged 4 commits intomainfrom
Conversation
… complex/null type tests PR #330 follow-up: the two uses of column.TypeName on lines 485 and 497 were inconsistent — the Arrow type mapping had no null guard while the metadata assignment used `?? string.Empty`. Extract a single `typeName` local to make null handling uniform and remove the silent divergence. Add two unit tests: - null TypeName falls back to StringType without throwing - DECIMAL(10,2), ARRAY, MAP, STRUCT are all mapped without exception Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
eric-wang-1990
had a problem deploying
to
azure-prod
GitHub Actions
Error
eric-wang-1990
changed the title
…complex type test The DECIMAL metadata value is already covered by ColumnMetadataHelperTests. Keep the Arrow type assertions (Decimal128Type, StringType) since those test MapDatabricksTypeToArrowType end-to-end, which is not exercised by the helper tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Re the |
eric-wang-1990
had a problem deploying
to
azure-prod
GitHub Actions
Error
| } | ||
|
|
||
| [Fact] | ||
| public async Task ExecuteQuery_EmptyTable_ComplexAndDecimalTypesAreMappedCorrectly() |
There was a problem hiding this comment.
Why need add these, we already have csharp/test/ColumnMetadataHelperTests.cs which test all type mapping?
There was a problem hiding this comment.
Good point — removed the redundant Spark:DataType:SqlName assertion for DECIMAL (already covered by ColumnMetadataHelperTests).
Kept the two tests because they cover things ColumnMetadataHelperTests doesn't:
ComplexAndDecimalTypesAreMappedCorrectly: exercisesMapDatabricksTypeToArrowTypeend-to-end (e.g.DECIMAL(10,2)→Decimal128Type,ARRAY→StringType) — the existingArrowTypesAreMappedCorrectlytest doesn't include these typesNullTypeName_TreatedAsUnknownType: verifies nullTypeNamedoesn't throw — not covered anywhere else
There was a problem hiding this comment.
You're right, removed it. The production change is just extracting a typeName local — the complex/decimal type mapping is out of scope.
There was a problem hiding this comment.
Moved it — added [InlineData("", "")] to the existing GetBaseTypeName and GetSparkSqlName theories in ColumnMetadataHelperTests and removed the statement-level test entirely.
Out of scope for this PR — the production change is just extracting a typeName local for null consistency, not touching type mapping logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
eric-wang-1990
had a problem deploying
to
azure-prod
GitHub Actions
Error
Add empty-string cases to GetBaseTypeName and GetSparkSqlName theories instead of a full statement-level integration test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
eric-wang-1990
temporarily deployed
to
azure-prod
GitHub Actions
Inactive
2 participants
PR #330 follow-up: the two uses of column.TypeName on lines 485 and 497 were inconsistent — the Arrow type mapping had no null guard while the metadata assignment used
?? string.Empty. Extract a singletypeNamelocal to make null handling uniform and remove the silent divergence.Add two unit tests:
What's Changed
Please fill in a description of the changes here.
This contains breaking changes.
Closes #NNN.