Conversation
|
Claude finished @khustup2's task in 1m 16s —— View job PR Review CompleteFound 4 critical issues that need attention:
These issues could prevent the feature from working correctly or cause crashes. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| case BYTEAOID: | ||
| case BYTEAOID: { | ||
| // Check for special domain types over BYTEA | ||
| if (pg::utils::is_file_domain_type(attr->atttypid)) { |
There was a problem hiding this comment.
Domain type check is applied to wrong OID - should check base type, not the domain OID itself. Domain types inherit from BYTEA (base type), but attr->atttypid holds the domain OID. When checking is_file_domain_type(attr->atttypid), it will match correctly, but the parent case BYTEAOID: will never trigger for domain types because PostgreSQL doesn't set atttypid to BYTEAOID for domains.
Fix: Need to either check the base type of the domain first, or restructure to check domain types before the base type switch. Consider:
// Before the switch statement, check for domains over BYTEA
Oid baseType = getBaseType(attr->atttypid);
if (baseType == BYTEAOID) {
if (pg::utils::is_file_domain_type(attr->atttypid)) { ... }
// etc.
}
| case BYTEAOID: | ||
| case BYTEAOID: { | ||
| // Check for special domain types over BYTEA | ||
| if (pg::utils::is_file_domain_type(attr->atttypid)) { |
There was a problem hiding this comment.
Same issue as extension_init.cpp:808 - domain type check won't work correctly because the case BYTEAOID: won't match domain types. Domain types have their own OID distinct from BYTEAOID.
Fix: Check the base type before entering the switch, or restructure to handle domains properly.
|
|
||
| Form_pg_type typTup = (Form_pg_type)GETSTRUCT(tup); | ||
|
|
||
| bool is_match = (typTup->typtype == TYPTYPE_DOMAIN && strcmp(NameStr(typTup->typname), domain_name) == 0); |
There was a problem hiding this comment.
Case-sensitive string comparison may cause issues if domains are created with different casing (e.g., "FILE" vs "file"). PostgreSQL identifiers are case-insensitive by default but stored in lowercase unless quoted.
Recommended fix: Use case-insensitive comparison or clarify that domains must be lowercase:
| bool is_match = (typTup->typtype == TYPTYPE_DOMAIN && strcmp(NameStr(typTup->typname), domain_name) == 0); | |
| bool is_match = (typTup->typtype == TYPTYPE_DOMAIN && pg_strcasecmp(NameStr(typTup->typname), domain_name) == 0); |
| case deeplake_core::type_kind::link: { | ||
| // Check if link contains bytes (file type) vs other types | ||
| auto inner = t.as_link().get_type(); | ||
| if (inner->is_generic() && inner->data_type().get_dtype() == nd::dtype::byte) { |
There was a problem hiding this comment.
Potential null pointer dereference if get_type() returns nullptr. Should add null check before dereferencing.
Fix:
| if (inner->is_generic() && inner->data_type().get_dtype() == nd::dtype::byte) { | |
| auto inner = t.as_link().get_type(); | |
| if (inner && inner->is_generic() && inner->data_type().get_dtype() == nd::dtype::byte) { |
|
1 participant
🚀 🚀 Pull Request
Impact
Description
Things to be aware of
Things to worry about
Additional Context