Skip to content

Commit

Permalink
sql: Fix generate_extracted_config! macro for rust-analyzer (#30004)
Browse files Browse the repository at this point in the history
This PR updates the `generate_extracted_config!` macro so rust-analyzer
no longer shows errors for invocations of the macro. The issue was
rust-analyzer was resolving traits implementations incorrectly so we
fully qualify some trait method calls to help it out. This makes the
macro a bit less flexible by requiring the `value` field of a SQL `WITH`
option be an `Option<WithOptionValue<T>>`, but all uses of it today are
exactly that type, so I don't anticipate any issue.

### Motivation

* This PR fixes a previously unreported bug.
rust-analyzer (used in IDEs) would show an error for invocations of this
macro, even though the code compiled without issue.

### Checklist

- [x] This PR has adequate test coverage / QA involvement has been duly
considered. ([trigger-ci for additional test/nightly
runs](https://trigger-ci.dev.materialize.com/))
- [x] This PR has an associated up-to-date [design
doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md),
is a design doc
([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)),
or is sufficiently small to not require a design.
  <!-- Reference the design in the description. -->
- [x] If this PR evolves [an existing `$T ⇔ Proto$T`
mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md)
(possibly in a backwards-incompatible way), then it is tagged with a
`T-proto` label.
- [x] If this PR will require changes to cloud orchestration or tests,
there is a companion cloud PR to account for those changes that is
tagged with the release-blocker label
([example](MaterializeInc/cloud#5021)).
<!-- Ask in #team-cloud on Slack if you need help preparing the cloud
PR. -->
- [x] If this PR includes major [user-facing behavior
changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note),
I have pinged the relevant PM to schedule a changelog post.
  • Loading branch information
ParkMyCar authored Oct 15, 2024
1 parent 43dcce4 commit 0f0d3e3
Show file tree
Hide file tree
Showing 5 changed files with 14 additions and 10 deletions.
18 changes: 12 additions & 6 deletions src/sql/src/normalize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,7 @@ macro_rules! generate_extracted_config {
if !$allow_multiple && !extracted.seen.insert(option.name.clone()) {
sql_bail!("{} specified more than once", option.name.to_ast_string());
}
let val = <$t>::try_from_value(option.value)
let val: $t = $crate::plan::with_options::TryFromValue::try_from_value(option.value)
.map_err(|e| sql_err!("invalid {}: {}", option.name.to_ast_string(), e))?;
generate_extracted_config!(
@ifexpr $allow_multiple,
Expand All @@ -616,18 +616,24 @@ macro_rules! generate_extracted_config {
let mut options = Vec::new();
$(
let value = self.[<$option_name:snake>];
let values = generate_extracted_config!(
let values: Vec<_> = generate_extracted_config!(
@ifexpr $allow_multiple,
value,
vec![value]
Vec::from([value])
);
for value in values {
// If `try_into_value` returns `None`, then there was no option that
// generated this value. For example, this can happen when `value` is
// `None`.
if let Some(value) = value.try_into_value(catalog) {
let option = $option_ty {name: $option_name, value};
options.push(option);
let maybe_value = <$t as $crate::plan::with_options::TryFromValue<
Option<mz_sql_parser::ast::WithOptionValue<$crate::names::Aug>>
>>::try_into_value(value, catalog);
match maybe_value {
Some(value) => {
let option = $option_ty {name: $option_name, value};
options.push(option);
},
None => (),
}
}
)*
Expand Down
1 change: 0 additions & 1 deletion src/sql/src/plan/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,6 @@ use crate::plan::plan_utils::{self, GroupSizeHints, JoinSide};
use crate::plan::scope::{Scope, ScopeItem, ScopeUngroupedColumn};
use crate::plan::statement::{show, StatementContext, StatementDesc};
use crate::plan::typeconv::{self, CastContext};
use crate::plan::with_options::TryFromValue;
use crate::plan::PlanError::InvalidWmrRecursionLimit;
use crate::plan::{
literal, transform_ast, Params, PlanContext, QueryWhen, ShowCreatePlan, WebhookValidation,
Expand Down
2 changes: 1 addition & 1 deletion src/sql/src/plan/statement/ddl/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ use mz_storage_types::connections::{

use crate::names::Aug;
use crate::plan::statement::{Connection, ResolvedItemName};
use crate::plan::with_options::{self, TryFromValue};
use crate::plan::with_options;
use crate::plan::{ConnectionDetails, PlanError, SshKey, StatementContext};
use crate::session::vars::{self, ENABLE_AWS_MSK_IAM_AUTH};

Expand Down
2 changes: 1 addition & 1 deletion src/sql/src/plan/statement/dml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ use crate::normalize;
use crate::plan::query::{plan_expr, plan_up_to, ExprContext, QueryLifetime};
use crate::plan::scope::Scope;
use crate::plan::statement::{ddl, StatementContext, StatementDesc};
use crate::plan::with_options::{self, TryFromValue};
use crate::plan::with_options;
use crate::plan::{
self, side_effecting_func, transform_ast, CopyToPlan, CreateSinkPlan, ExplainPushdownPlan,
ExplainSinkSchemaPlan, ExplainTimestampPlan,
Expand Down
1 change: 0 additions & 1 deletion src/sql/src/plan/statement/scl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ use crate::ast::{
};
use crate::names::{self, Aug};
use crate::plan::statement::{StatementContext, StatementDesc};
use crate::plan::with_options::TryFromValue;
use crate::plan::{
describe, query, ClosePlan, DeallocatePlan, DeclarePlan, ExecutePlan, ExecuteTimeout,
FetchPlan, InspectShardPlan, Params, Plan, PlanError, PreparePlan, ResetVariablePlan,
Expand Down

0 comments on commit 0f0d3e3

Please sign in to comment.