Skip to content

Commit

Permalink
perf(cubesql): Improve LogicalPlanLanguage parsing
Browse files Browse the repository at this point in the history 
* Add separate cheaper error type, without backtrace and input part
* Avoid run-time formatting to add :
* Avoid calls to replace when strip_prefix + to_string is enough
  • Loading branch information
@mcheshkov
mcheshkov committed Dec 4, 2024
1 parent fc8549c commit 44cfefa
Show file tree
Hide file tree
Showing 2 changed files with 105 additions and 74 deletions.
170 changes: 102 additions & 68 deletions rust/cubesql/cubesql/src/compile/rewrite/language.rs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,34 @@
use std::{
num::{ParseFloatError, ParseIntError},
str::ParseBoolError,
};

// `from_op` is generated as if-else chain from op.parse()
// Because of this alot of instances of FromStr::Err wil lbe constructed just as a "take other branch" marker
// This type should be very cheap to construct new instances, at least while we use FromStr chains
// Don't store any input in here, FromStr is not designed for this, and it requires allocation
// Instead rely on egg::FromOpError for now, it will return allocated `op`, but only once in the end
// TODO make parser dispatch stricter, and return both input and LanguageParseError from `from_op`
#[derive(thiserror::Error, Debug)]

Check warning on line 12 in rust/cubesql/cubesql/src/compile/rewrite/language.rs

View check run for this annotation

Codecov / codecov/patch

rust/cubesql/cubesql/src/compile/rewrite/language.rs#L12 

Added line #L12 was not covered by tests
pub enum LanguageParseError {
#[error("Should start with '{0}'")]
ShouldStartWith(&'static str),
#[error("Can't be matched against {0}")]
ShouldMatch(&'static str),
#[error("Should contain a valid type")]
InvalidType,
#[error("Should contains a valid scalar type")]
InvalidScalarType,
#[error("Can't parse boolean scalar value with error: {0}")]
InvalidBoolValue(#[source] ParseBoolError),
#[error("Can't parse i64 scalar value with error: {0}")]
InvalidIntValue(#[source] ParseIntError),
#[error("Can't parse f64 scalar value with error: {0}")]
InvalidFloatValue(#[source] ParseFloatError),
#[error("Conversion from string is not supported")]
NotSupported,
}

#[macro_export]
macro_rules! plan_to_language {
($(#[$meta:meta])* $vis:vis enum $name:ident $variants:tt) => {
Expand All @@ -13,13 +44,13 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](String);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let prefix = format!("{}:", std::stringify!([<$variant $var_field:camel>]));
if s.starts_with(&prefix) {
return Ok([<$variant $var_field:camel>](s.replace(&prefix, "")));
const PREFIX: &'static str = concat!(std::stringify!([<$variant $var_field:camel>]), ":");
if let Some(suffix) = s.strip_prefix(PREFIX) {
return Ok([<$variant $var_field:camel>](suffix.to_string()));
}
Err(CubeError::internal(format!("Can't convert {}. Should start with '{}'", s, prefix)))
Err(Self::Err::ShouldStartWith(PREFIX))
}
}

Expand All @@ -37,13 +68,13 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](usize);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let prefix = format!("{}:", std::stringify!([<$variant $var_field:camel>]));
if s.starts_with(&prefix) {
return Ok([<$variant $var_field:camel>](s.replace(&prefix, "").parse().unwrap()));
const PREFIX: &'static str = concat!(std::stringify!([<$variant $var_field:camel>]), ":");
if let Some(suffix) = s.strip_prefix(PREFIX) {
return Ok([<$variant $var_field:camel>](suffix.parse().unwrap()));

Check warning on line 75 in rust/cubesql/cubesql/src/compile/rewrite/language.rs

View check run for this annotation

Codecov / codecov/patch

rust/cubesql/cubesql/src/compile/rewrite/language.rs#L75 

Added line #L75 was not covered by tests
}
Err(CubeError::internal(format!("Can't convert {}. Should start with '{}'", s, prefix)))
Err(Self::Err::ShouldStartWith(PREFIX))
}
}

Expand All @@ -61,13 +92,13 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](bool);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let prefix = format!("{}:", std::stringify!([<$variant $var_field:camel>]));
if s.starts_with(&prefix) {
return Ok([<$variant $var_field:camel>](s.replace(&prefix, "").parse().unwrap()));
const PREFIX: &'static str = concat!(std::stringify!([<$variant $var_field:camel>]), ":");
if let Some(suffix) = s.strip_prefix(PREFIX) {
return Ok([<$variant $var_field:camel>](suffix.parse().unwrap()));
}
Err(CubeError::internal(format!("Can't convert {}. Should start with '{}'", s, prefix)))
Err(Self::Err::ShouldStartWith(PREFIX))
}
}

Expand All @@ -85,18 +116,17 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](Option<usize>);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let prefix = format!("{}:", std::stringify!([<$variant $var_field:camel>]));
if s.starts_with(&prefix) {
let replaced = s.replace(&prefix, "");
if &replaced == "None" {
const PREFIX: &'static str = concat!(std::stringify!([<$variant $var_field:camel>]), ":");
if let Some(suffix) = s.strip_prefix(PREFIX) {
if suffix == "None" {
return Ok([<$variant $var_field:camel>](None));
} else {
return Ok([<$variant $var_field:camel>](Some(s.replace(&prefix, "").parse().unwrap())));
return Ok([<$variant $var_field:camel>](Some(suffix.parse().unwrap())));
}
}
Err(CubeError::internal(format!("Can't convert {}. Should start with '{}'", s, prefix)))
Err(Self::Err::ShouldStartWith(PREFIX))
}
}

Expand All @@ -114,18 +144,17 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](Option<Vec<String>>);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let prefix = format!("{}:", std::stringify!([<$variant $var_field:camel>]));
if s.starts_with(&prefix) {
let replaced = s.replace(&prefix, "");
if &replaced == "None" {
const PREFIX: &'static str = concat!(std::stringify!([<$variant $var_field:camel>]), ":");
if let Some(suffix) = s.strip_prefix(PREFIX) {
if suffix == "None" {

Check warning on line 151 in rust/cubesql/cubesql/src/compile/rewrite/language.rs

View check run for this annotation

Codecov / codecov/patch

rust/cubesql/cubesql/src/compile/rewrite/language.rs#L151 

Added line #L151 was not covered by tests
return Ok([<$variant $var_field:camel>](None));
} else {
return Ok([<$variant $var_field:camel>](Some(s.split(',').map(|s| s.to_string()).collect::<Vec<_>>())));
return Ok([<$variant $var_field:camel>](Some(suffix.split(',').map(|s| s.to_string()).collect::<Vec<_>>())));

Check warning on line 154 in rust/cubesql/cubesql/src/compile/rewrite/language.rs

View check run for this annotation

Codecov / codecov/patch

rust/cubesql/cubesql/src/compile/rewrite/language.rs#L154 

Added line #L154 was not covered by tests
}
}
Err(CubeError::internal(format!("Can't convert {}. Should start with '{}'", s, prefix)))
Err(Self::Err::ShouldStartWith(PREFIX))
}
}

Expand All @@ -143,18 +172,17 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](Option<String>);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let prefix = format!("{}:", std::stringify!([<$variant $var_field:camel>]));
if s.starts_with(&prefix) {
let replaced = s.replace(&prefix, "");
if &replaced == "None" {
const PREFIX: &'static str = concat!(std::stringify!([<$variant $var_field:camel>]), ":");
if let Some(suffix) = s.strip_prefix(PREFIX) {
if suffix == "None" {
return Ok([<$variant $var_field:camel>](None));
} else {
return Ok([<$variant $var_field:camel>](Some(s.to_string())));
return Ok([<$variant $var_field:camel>](Some(suffix.to_string())));

Check warning on line 182 in rust/cubesql/cubesql/src/compile/rewrite/language.rs

View check run for this annotation

Codecov / codecov/patch

rust/cubesql/cubesql/src/compile/rewrite/language.rs#L182 

Added line #L182 was not covered by tests
}
}
Err(CubeError::internal("Conversion from string is not supported".to_string()))
Err(Self::Err::NotSupported)
}
}

Expand All @@ -172,9 +200,9 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](Column);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(_s: &str) -> Result<Self, Self::Err> {
Err(CubeError::internal("Conversion from string is not supported".to_string()))
Err(Self::Err::NotSupported)
}
}

Expand All @@ -192,9 +220,9 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](Vec<Column>);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(_s: &str) -> Result<Self, Self::Err> {
Err(CubeError::internal("Conversion from string is not supported".to_string()))
Err(Self::Err::NotSupported)
}
}

Expand All @@ -212,9 +240,9 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](Vec<Column>);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(_s: &str) -> Result<Self, Self::Err> {
Err(CubeError::internal("Conversion from string is not supported".to_string()))
Err(Self::Err::NotSupported)
}
}

Expand All @@ -232,9 +260,9 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](String);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(_s: &str) -> Result<Self, Self::Err> {
Err(CubeError::internal("Conversion from string is not supported".to_string()))
Err(Self::Err::NotSupported)
}
}

Expand All @@ -252,9 +280,9 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](String);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(_s: &str) -> Result<Self, Self::Err> {
Err(CubeError::internal("Conversion from string is not supported".to_string()))
Err(Self::Err::NotSupported)
}
}

Expand All @@ -272,13 +300,13 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](String);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let prefix = format!("{}:", std::stringify!([<$variant $var_field:camel>]));
if s.starts_with(&prefix) {
return Ok([<$variant $var_field:camel>](s.replace(&prefix, "")));
const PREFIX: &'static str = concat!(std::stringify!([<$variant $var_field:camel>]), ":");
if let Some(suffix) = s.strip_prefix(PREFIX) {
return Ok([<$variant $var_field:camel>](suffix.to_string()));
}
Err(CubeError::internal(format!("Can't convert {}. Should start with '{}'", s, prefix)))
Err(Self::Err::ShouldStartWith(PREFIX))
}
}

Expand Down Expand Up @@ -484,14 +512,16 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>]($var_field_type);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let prefix = format!("{}:", std::stringify!([<$variant $var_field:camel>]));
let name = s.strip_prefix(&prefix).ok_or(CubeError::internal(format!("Can't convert {}. Should start with '{}'", s, prefix)))?;
const PREFIX: &'static str = concat!(std::stringify!([<$variant $var_field:camel>]), ":");
let name = s
.strip_prefix(PREFIX)
.ok_or(Self::Err::ShouldStartWith(PREFIX))?;

match name {
$($name => Ok([<$variant $var_field:camel>]($variant_type)),)*
x => Err(CubeError::internal(format!("{} can't be matched against {}", x, std::stringify!($var_field_type))))
_ => Err(Self::Err::ShouldMatch(std::stringify!($var_field_type)))

Check warning on line 524 in rust/cubesql/cubesql/src/compile/rewrite/language.rs

View check run for this annotation

Codecov / codecov/patch

rust/cubesql/cubesql/src/compile/rewrite/language.rs#L524 

Added line #L524 was not covered by tests
}
}
}
Expand Down Expand Up @@ -557,10 +587,12 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](DataType);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let prefix = format!("{}:", std::stringify!([<$variant $var_field:camel>]));
let typed_str = s.strip_prefix(&prefix).ok_or(CubeError::internal(format!("Can't convert {}. Should start with '{}'", s, prefix)))?;
const PREFIX: &'static str = concat!(std::stringify!([<$variant $var_field:camel>]), ":");
let typed_str = s
.strip_prefix(PREFIX)
.ok_or(Self::Err::ShouldStartWith(PREFIX))?;

match typed_str {
"Float32" => Ok([<$variant $var_field:camel>](DataType::Float32)),
Expand All @@ -571,7 +603,7 @@ macro_rules! variant_field_struct {
"Utf8" => Ok([<$variant $var_field:camel>](DataType::Utf8)),
"Date32" => Ok([<$variant $var_field:camel>](DataType::Date32)),
"Date64" => Ok([<$variant $var_field:camel>](DataType::Date64)),
_ => Err(CubeError::internal(format!("Can't convert {}. Should contain a valid type, actual: {}", s, typed_str))),
_ => Err(Self::Err::InvalidType),

Check warning on line 606 in rust/cubesql/cubesql/src/compile/rewrite/language.rs

View check run for this annotation

Codecov / codecov/patch

rust/cubesql/cubesql/src/compile/rewrite/language.rs#L606 

Added line #L606 was not covered by tests
}
}
}
Expand All @@ -590,24 +622,26 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>](ScalarValue);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(s: &str) -> Result<Self, Self::Err> {
let prefix = format!("{}:", std::stringify!([<$variant $var_field:camel>]));
let typed_str = s.strip_prefix(&prefix).ok_or(CubeError::internal(format!("Can't convert {}. Should start with '{}'", s, prefix)))?;
const PREFIX: &'static str = concat!(std::stringify!([<$variant $var_field:camel>]), ":");
let typed_str = s
.strip_prefix(PREFIX)
.ok_or(Self::Err::ShouldStartWith(PREFIX))?;

if let Some(value) = typed_str.strip_prefix("s:") {
Ok([<$variant $var_field:camel>](ScalarValue::Utf8(Some(value.to_string()))))
} else if let Some(value) = typed_str.strip_prefix("b:") {
let n: bool = value.parse().map_err(|err| CubeError::internal(format!("Can't parse boolean scalar value from '{}' with error: {}", typed_str, err)))?;
let n: bool = value.parse().map_err(|err| Self::Err::InvalidBoolValue(err))?;
Ok([<$variant $var_field:camel>](ScalarValue::Boolean(Some(n))))
} else if let Some(value) = typed_str.strip_prefix("i:") {
let n: i64 = value.parse().map_err(|err| CubeError::internal(format!("Can't parse i64 scalar value from '{}' with error: {}", typed_str, err)))?;
let n: i64 = value.parse().map_err(|err| Self::Err::InvalidIntValue(err))?;
Ok([<$variant $var_field:camel>](ScalarValue::Int64(Some(n))))
} else if let Some(value) = typed_str.strip_prefix("f:") {
let n: f64 = value.parse().map_err(|err| CubeError::internal(format!("Can't parse f64 scalar value from '{}' with error: {}", typed_str, err)))?;
let n: f64 = value.parse().map_err(|err| Self::Err::InvalidFloatValue(err))?;
Ok([<$variant $var_field:camel>](ScalarValue::Float64(Some(n))))
} else {
Err(CubeError::internal(format!("Can't convert {}. Should contains type type, actual: {}", s, typed_str)))
Err(Self::Err::InvalidScalarType)

Check warning on line 644 in rust/cubesql/cubesql/src/compile/rewrite/language.rs

View check run for this annotation

Codecov / codecov/patch

rust/cubesql/cubesql/src/compile/rewrite/language.rs#L644 

Added line #L644 was not covered by tests
}
}
}
Expand Down Expand Up @@ -657,9 +691,9 @@ macro_rules! variant_field_struct {
pub struct [<$variant $var_field:camel>]($var_field_type);

impl FromStr for [<$variant $var_field:camel>] {
type Err = CubeError;
type Err = $crate::compile::rewrite::language::LanguageParseError;
fn from_str(_s: &str) -> Result<Self, Self::Err> {
Err(CubeError::internal("Conversion from string is not supported".to_string()))
Err(Self::Err::NotSupported)
}
}

Expand Down
9 changes: 3 additions & 6 deletions rust/cubesql/cubesql/src/compile/rewrite/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,9 @@ pub mod rewriter;
pub mod rules;

use self::analysis::{LogicalPlanData, MemberNameToExpr};
use crate::{
compile::rewrite::{
analysis::{LogicalPlanAnalysis, OriginalExpr},
rewriter::{CubeEGraph, CubeRewrite},
},
CubeError,
use crate::compile::rewrite::{
analysis::{LogicalPlanAnalysis, OriginalExpr},
rewriter::{CubeEGraph, CubeRewrite},
};
use analysis::MemberNamesToExpr;
use datafusion::{
Expand Down

0 comments on commit 44cfefa

Please sign in to comment.