Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

edit: datum struct string type added utf8 check #1488

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

edit: datum struct string type added utf8 check #1488

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b80ff4e
edit: datum string type Added utf8 check
dust1 Feb 26, 2024
73ebbd0
Merge branch 'main' into issue1300
dust1 Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion src/common_types/src/datum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@

//! Datum holds different kind of data

use std::{convert::TryFrom, fmt, str};
use std::{
convert::TryFrom,
fmt,
str::{self, from_utf8, Utf8Error},
};

use arrow::{
datatypes::{DataType, TimeUnit},
Expand Down Expand Up @@ -102,6 +106,13 @@ pub enum Error {
hex_val: String,
backtrace: Backtrace,
},

#[snafu(display("Invalid string encoding, that are not utf-8 characters input:{msg}, err:{source}. \nBacktrace:\n{backtrace}"))]
InvalidStringEncoding {
msg: String,
source: Utf8Error,
backtrace: Backtrace,
},
}

pub type Result<T> = std::result::Result<T, Error>;
Expand Down Expand Up @@ -765,6 +776,11 @@ impl Datum {
}
}

fn valid_is_utf8(s: &str) -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check may be expensive for long string, better to add an option to decide whether to do this check.

from_utf8(s.as_bytes()).context(InvalidStringEncoding { msg: s })?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this function should return bool, not a result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Ok(())
}

pub fn try_from_sql_value(kind: &DatumKind, value: Value) -> Result<Datum> {
match (kind, value) {
(DatumKind::Null, Value::Null) => Ok(Datum::Null),
Expand All @@ -790,19 +806,23 @@ impl Datum {
}
// TODO(yingwen): Support hex string.
(DatumKind::Varbinary, Value::SingleQuotedString(s)) => {
Self::valid_is_utf8(&s)?;
Ok(Datum::Varbinary(Bytes::from(s)))
}
(DatumKind::String, Value::SingleQuotedString(s)) => {
Self::valid_is_utf8(&s)?;
Ok(Datum::String(StringBytes::from(s)))
}
(DatumKind::Varbinary, Value::DoubleQuotedString(s)) => {
Self::valid_is_utf8(&s)?;
Ok(Datum::Varbinary(Bytes::from(s)))
}
(DatumKind::Varbinary, Value::HexStringLiteral(s)) => {
let bytes = hex::try_decode(&s).context(InvalidHexValue { hex_val: s })?;
Ok(Datum::Varbinary(Bytes::from(bytes)))
}
(DatumKind::String, Value::DoubleQuotedString(s)) => {
Self::valid_is_utf8(&s)?;
Ok(Datum::String(StringBytes::from(s)))
}
(DatumKind::UInt64, Value::Number(n, _long)) => {
Expand Down
Loading