YQ improved s3 read / write partitions validation #20081
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🔴 Bugfix requires a linked issue in the changelog entry |
There was a problem hiding this comment.
Pull Request Overview
This PR improves S3 read/write partition validation by enhancing error messages and adding extra validation checks on schema and partition definitions. Key changes include:
- Refining error messages in the S3 datasink type annotation and path generator to display more specific details.
- Replacing deprecated ythrow syntax with standard throw for consistency.
- Adding new unit tests to validate raw format and partitioned-by scenarios, and updating external source validation for partitioned columns.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ydb/library/yql/providers/s3/provider/yql_s3_datasink_type_ann.cpp | Improved error message by including the missing key’s content and added a validation check for write schema columns. |
| ydb/library/yql/providers/s3/path_generator/yql_s3_path_generator.cpp | Replaced ythrow with throw and refined error messages for improved clarity on projection and storage key validations. |
| ydb/library/yql/providers/s3/common/util.cpp | Updated raw format schema validation to explicitly check for a primitive data type. |
| ydb/core/kqp/ut/federated_query/s3/kqp_federated_query_ut.cpp | Added unit tests for raw format insert validation and partitioned-by schema validation. |
| ydb/core/external_sources/object_storage_ut.cpp | Introduced additional unit tests to cover failed partitioned_by validations. |
| ydb/core/external_sources/object_storage.cpp | Enhanced partitioned_by parsing with explicit type checks and a try-catch block for improved error reporting. |
Comments suppressed due to low confidence (2)
ydb/library/yql/providers/s3/path_generator/yql_s3_path_generator.cpp:357
- [nitpick] Consider ending the error message with a period and confirming that its phrasing matches other similar messages in the file to maintain consistency.
throw yexception() << "Partition by must always be specified with projection";
ydb/library/yql/providers/s3/common/util.cpp:223
- [nitpick] Consider listing the supported primitive types in the error message so that users know which types are acceptable for raw format.
ctx.AddError(TIssue(ctx.GetPosition(pos), TStringBuilder() << "Only column with primitive type allowed for raw format (you have field with type " << *rowType << ")"));
ydb/library/yql/providers/s3/provider/yql_s3_datasink_type_ann.cpp
Outdated
Show resolved
Hide resolved
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
…tor.cpp Co-authored-by: Copilot <[email protected]>
….cpp Co-authored-by: Copilot <[email protected]>
GrigoriyPA
force-pushed
the
YQ-fix-settings-validation
branch
from
4d84c71 to
10901e2
Compare
|
⚪ Test history | Ya make output | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
Changelog entry
Improved s3 read / write partitions validation
Changelog category
Description for reviewers