Skip to content

fix: expand DataValue::Utf8 so that it can correspond to Char and Varchar respectively.#183

Merged
KKould merged 3 commits intoKipData:mainfrom
KKould:main
Mar 26, 2024
Merged

fix: expand DataValue::Utf8 so that it can correspond to Char and Varchar respectively.#183
KKould merged 3 commits intoKipData:mainfrom
KKould:main

Conversation

@KKould
Copy link
Member

@KKould KKould commented Mar 26, 2024

What problem does this PR solve?

originally, both LogicalType::Char and LogicalType::Varchar used DataValue::Utf8, but DataValue::Utf8 does not know its own logical_type

Code changes

  • Has Rust code change
  • Has CI related scripts change

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Note for reviewer

@KKould KKould requested a review from crwen March 26, 2024 11:12
@KKould KKould added the invalid This doesn't seem right label Mar 26, 2024
@KKould KKould self-assigned this Mar 26, 2024
@crwen
Copy link
Member

crwen commented Mar 26, 2024

Is this a necessary change? can you give me some situations

@KKould
Copy link
Member Author

KKould commented Mar 26, 2024

Is this a necessary change? can you give me some situations

This pr is mainly due to the DataValue::logical_type method, because when selecting LogicalType::Char and LogicalType::Varchar to share DataValue::Utf8 in the previous pr: #174, the conversion from DataValue to LogicalType was ignored, which may cause LogicalType::Char to be converted to DataValue:: Utf8 and then converted to LogicalType::Varchar, potentially causing many problems

@KKould KKould merged commit 5ed5ce4 into KipData:main Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

invalid This doesn't seem right

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants