Skip to content

Commit a9d4a89

Browse files
CodeBlanchCodeBlanch
authored
[query-engine] Recordset engine diagnostics/logging improvements (open-telemetry#1369)
## Changes * Clean up the code around "Resolved as:" logging to have less duplication * Maps and Arrays are no longer logged as JSON strings (to prevent bloat) * Strings will only be logged up to 32 characters (to prevent bloat) * Add type information. Instead of `'null'` (which could be a `null` value or a string of `"null"`) we will now get `[Null]` or `[String("null")]`.
1 parent aa23e71 commit a9d4a89

12 files changed

+298
-463
lines changed

‎rust/experimental/query_engine/engine-recordset/src/logical_expressions.rs‎

Lines changed: 26 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -9,171 +9,93 @@ pub fn execute_logical_expression<'a, TRecord: Record>(
99
execution_context: &ExecutionContext<'a, '_, '_, TRecord>,
1010
logical_expression: &'a LogicalExpression,
1111
) -> Result<bool, ExpressionError> {
12-
match logical_expression {
12+
let value = match logical_expression {
1313
LogicalExpression::Scalar(s) => {
1414
let value = execute_scalar_expression(execution_context, s)?;
1515

1616
if let Some(b) = value.to_value().convert_to_bool() {
17-
execution_context.add_diagnostic_if_enabled(
18-
RecordSetEngineDiagnosticLevel::Verbose,
19-
logical_expression,
20-
|| format!("Evaluated as: '{b}'"),
21-
);
22-
Ok(b)
17+
b
2318
} else {
24-
Err(ExpressionError::TypeMismatch(
19+
return Err(ExpressionError::TypeMismatch(
2520
s.get_query_location().clone(),
2621
format!(
2722
"Value of '{:?}' type returned by scalar expression could not be converted to bool",
2823
value.get_value_type()
2924
),
30-
))
25+
));
3126
}
3227
}
3328
LogicalExpression::EqualTo(e) => {
3429
let left = execute_scalar_expression(execution_context, e.get_left())?;
3530

3631
let right = execute_scalar_expression(execution_context, e.get_right())?;
3732

38-
match Value::are_values_equal(
33+
Value::are_values_equal(
3934
e.get_query_location(),
4035
&left.to_value(),
4136
&right.to_value(),
4237
e.get_case_insensitive(),
43-
) {
44-
Ok(b) => {
45-
execution_context.add_diagnostic_if_enabled(
46-
RecordSetEngineDiagnosticLevel::Verbose,
47-
logical_expression,
48-
|| format!("Evaluated as: '{b}'"),
49-
);
50-
Ok(b)
51-
}
52-
Err(e) => Err(e),
53-
}
38+
)?
5439
}
5540
LogicalExpression::GreaterThan(g) => {
5641
let left = execute_scalar_expression(execution_context, g.get_left())?;
5742

5843
let right = execute_scalar_expression(execution_context, g.get_right())?;
5944

60-
match Value::compare_values(g.get_query_location(), &left.to_value(), &right.to_value())
61-
{
62-
Ok(v) => {
63-
let r = v > 0;
64-
execution_context.add_diagnostic_if_enabled(
65-
RecordSetEngineDiagnosticLevel::Verbose,
66-
logical_expression,
67-
|| format!("Evaluated as: '{r}'"),
68-
);
69-
Ok(r)
70-
}
71-
Err(e) => Err(e),
72-
}
45+
Value::compare_values(g.get_query_location(), &left.to_value(), &right.to_value())? > 0
7346
}
7447
LogicalExpression::GreaterThanOrEqualTo(g) => {
7548
let left = execute_scalar_expression(execution_context, g.get_left())?;
7649

7750
let right = execute_scalar_expression(execution_context, g.get_right())?;
7851

79-
match Value::compare_values(g.get_query_location(), &left.to_value(), &right.to_value())
80-
{
81-
Ok(v) => {
82-
let r = v >= 0;
83-
execution_context.add_diagnostic_if_enabled(
84-
RecordSetEngineDiagnosticLevel::Verbose,
85-
logical_expression,
86-
|| format!("Evaluated as: '{r}'"),
87-
);
88-
Ok(r)
89-
}
90-
Err(e) => Err(e),
91-
}
52+
Value::compare_values(g.get_query_location(), &left.to_value(), &right.to_value())? >= 0
9253
}
9354
LogicalExpression::Not(n) => {
94-
match execute_logical_expression(execution_context, n.get_inner_expression()) {
95-
Ok(mut b) => {
96-
b = !b;
97-
execution_context.add_diagnostic_if_enabled(
98-
RecordSetEngineDiagnosticLevel::Verbose,
99-
logical_expression,
100-
|| format!("Evaluated as: '{b}'"),
101-
);
102-
Ok(b)
103-
}
104-
Err(e) => Err(e),
105-
}
55+
!execute_logical_expression(execution_context, n.get_inner_expression())?
10656
}
10757
LogicalExpression::And(a) => {
108-
let result = match execute_logical_expression(execution_context, a.get_left())? {
58+
match execute_logical_expression(execution_context, a.get_left())? {
10959
false => false,
11060
true => execute_logical_expression(execution_context, a.get_right())?,
111-
};
112-
113-
execution_context.add_diagnostic_if_enabled(
114-
RecordSetEngineDiagnosticLevel::Verbose,
115-
logical_expression,
116-
|| format!("Evaluated as: '{result}'"),
117-
);
118-
119-
Ok(result)
61+
}
12062
}
12163
LogicalExpression::Or(o) => {
122-
let result = match execute_logical_expression(execution_context, o.get_left())? {
64+
match execute_logical_expression(execution_context, o.get_left())? {
12365
true => true,
12466
false => execute_logical_expression(execution_context, o.get_right())?,
125-
};
126-
127-
execution_context.add_diagnostic_if_enabled(
128-
RecordSetEngineDiagnosticLevel::Verbose,
129-
logical_expression,
130-
|| format!("Evaluated as: '{result}'"),
131-
);
132-
133-
Ok(result)
67+
}
13468
}
13569
LogicalExpression::Contains(c) => {
13670
let haystack = execute_scalar_expression(execution_context, c.get_haystack())?;
13771
let needle = execute_scalar_expression(execution_context, c.get_needle())?;
13872

139-
match Value::contains(
73+
Value::contains(
14074
c.get_query_location(),
14175
&haystack.to_value(),
14276
&needle.to_value(),
14377
c.get_case_insensitive(),
144-
) {
145-
Ok(b) => {
146-
execution_context.add_diagnostic_if_enabled(
147-
RecordSetEngineDiagnosticLevel::Verbose,
148-
logical_expression,
149-
|| format!("Evaluated as: '{b}'"),
150-
);
151-
Ok(b)
152-
}
153-
Err(e) => Err(e),
154-
}
78+
)?
15579
}
15680
LogicalExpression::Matches(m) => {
15781
let haystack = execute_scalar_expression(execution_context, m.get_haystack())?;
15882
let pattern = execute_scalar_expression(execution_context, m.get_pattern())?;
15983

160-
match Value::matches(
84+
Value::matches(
16185
m.get_query_location(),
16286
&haystack.to_value(),
16387
&pattern.to_value(),
164-
) {
165-
Ok(b) => {
166-
execution_context.add_diagnostic_if_enabled(
167-
RecordSetEngineDiagnosticLevel::Verbose,
168-
logical_expression,
169-
|| format!("Evaluated as: '{b}'"),
170-
);
171-
Ok(b)
172-
}
173-
Err(e) => Err(e),
174-
}
88+
)?
17589
}
176-
}
90+
};
91+
92+
execution_context.add_diagnostic_if_enabled(
93+
RecordSetEngineDiagnosticLevel::Verbose,
94+
logical_expression,
95+
|| format!("Evaluated as: {value}"),
96+
);
97+
98+
Ok(value)
17799
}
178100

179101
#[cfg(test)]

‎rust/experimental/query_engine/engine-recordset/src/primitives/resolved_value.rs‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,38 @@ impl AsValue for ResolvedValue<'_> {
286286

287287
impl Display for ResolvedValue<'_> {
288288
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
289-
self.to_value().fmt(f)
289+
f.write_str("[")?;
290+
match self.to_value() {
291+
Value::Null => f.write_str("Null"),
292+
Value::Array(a) => {
293+
write!(f, "Array(Count={})", a.len())
294+
}
295+
Value::Map(m) => {
296+
write!(f, "Map(Count={})", m.len())
297+
}
298+
Value::String(s) => {
299+
f.write_str("String(")?;
300+
let v = s.get_value();
301+
if v.len() <= 32 {
302+
f.write_str(serde_json::to_string(&v).unwrap().as_str())?;
303+
} else {
304+
write!(
305+
f,
306+
"{}",
307+
serde_json::to_string(&format!("{}...", &v[..32]))
308+
.unwrap()
309+
.as_str()
310+
)?;
311+
}
312+
f.write_str(")")
313+
}
314+
v => {
315+
write!(f, "{}(", v.get_value_type())?;
316+
v.fmt(f)?;
317+
f.write_str(")")
318+
}
319+
}?;
320+
f.write_str("]")
290321
}
291322
}
292323

‎rust/experimental/query_engine/engine-recordset/src/scalars/collection_scalar_expressions.rs‎

Lines changed: 20 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ where
1313
'a: 'c,
1414
'b: 'c,
1515
{
16-
match collection_scalar_expression {
16+
let value = match collection_scalar_expression {
1717
CollectionScalarExpression::Concat(c) => {
1818
match execute_scalar_expression(execution_context, c.get_values_expression())?
1919
.try_resolve_array()
@@ -34,23 +34,17 @@ where
3434
}
3535
})?;
3636

37-
let r = ResolvedValue::Sequence(Sequence::new(values));
38-
39-
execution_context.add_diagnostic_if_enabled(
40-
RecordSetEngineDiagnosticLevel::Verbose,
41-
collection_scalar_expression,
42-
|| format!("Evaluated as: '{r}'"),
43-
);
44-
45-
Ok(r)
37+
ResolvedValue::Sequence(Sequence::new(values))
38+
}
39+
Err(orig) => {
40+
return Err(ExpressionError::TypeMismatch(
41+
c.get_values_expression().get_query_location().clone(),
42+
format!(
43+
"Value of '{:?}' type returned by scalar expression was not an array",
44+
orig.get_value_type()
45+
),
46+
));
4647
}
47-
Err(orig) => Err(ExpressionError::TypeMismatch(
48-
c.get_values_expression().get_query_location().clone(),
49-
format!(
50-
"Value of '{:?}' type returned by scalar expression was not an array",
51-
orig.get_value_type()
52-
),
53-
)),
5448
}
5549
}
5650
CollectionScalarExpression::List(c) => {
@@ -62,17 +56,17 @@ where
6256
values.push(execute_scalar_expression(execution_context, v)?);
6357
}
6458

65-
let r = ResolvedValue::List(List::new(values));
59+
ResolvedValue::List(List::new(values))
60+
}
61+
};
6662

67-
execution_context.add_diagnostic_if_enabled(
68-
RecordSetEngineDiagnosticLevel::Verbose,
69-
collection_scalar_expression,
70-
|| format!("Evaluated as: '{r}'"),
71-
);
63+
execution_context.add_diagnostic_if_enabled(
64+
RecordSetEngineDiagnosticLevel::Verbose,
65+
collection_scalar_expression,
66+
|| format!("Evaluated as: {value}"),
67+
);
7268

73-
Ok(r)
74-
}
75-
}
69+
Ok(value)
7670
}
7771

7872
#[cfg(test)]

‎rust/experimental/query_engine/engine-recordset/src/scalars/convert_scalar_expressions.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ where
147147
execution_context.add_diagnostic_if_enabled(
148148
RecordSetEngineDiagnosticLevel::Verbose,
149149
convert_scalar_expression,
150-
|| format!("Evaluated as: '{value}'"),
150+
|| format!("Evaluated as: {value}"),
151151
);
152152

153153
Ok(value)

‎rust/experimental/query_engine/engine-recordset/src/scalars/math_scalar_expressions.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ where
4242
execution_context.add_diagnostic_if_enabled(
4343
RecordSetEngineDiagnosticLevel::Verbose,
4444
math_scalar_expression,
45-
|| format!("Evaluated as: '{value}'"),
45+
|| format!("Evaluated as: {value}"),
4646
);
4747

4848
Ok(value)

‎rust/experimental/query_engine/engine-recordset/src/scalars/parse_scalar_expressions.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ where
1313
'a: 'c,
1414
'b: 'c,
1515
{
16-
let v = match parse_scalar_expression {
16+
let value = match parse_scalar_expression {
1717
ParseScalarExpression::Json(p) => {
1818
let inner_value =
1919
execute_scalar_expression(execution_context, p.get_inner_expression())?;
@@ -99,10 +99,10 @@ where
9999
execution_context.add_diagnostic_if_enabled(
100100
RecordSetEngineDiagnosticLevel::Verbose,
101101
parse_scalar_expression,
102-
|| format!("Evaluated as: {v}"),
102+
|| format!("Evaluated as: {value}"),
103103
);
104104

105-
Ok(v)
105+
Ok(value)
106106
}
107107

108108
#[cfg(test)]

0 commit comments

Comments
 (0)