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.

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

prune superfluous inferred object types from native query configuration #118

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
21 changes: 11 additions & 10 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ ndc-models = { workspace = true }
nom = "^7.1.3"
nonempty = "^0.10.0"
ref-cast = { workspace = true }
regex = "^1.11.1"
serde = { workspace = true }
serde_json = { workspace = true }
thiserror = "1.0.57"
Expand Down
32 changes: 32 additions & 0 deletions crates/cli/src/native_query/helpers.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
use std::{borrow::Cow, collections::BTreeMap};

use configuration::Configuration;
use ndc_models::{CollectionInfo, CollectionName, ObjectTypeName};
use regex::Regex;

use super::error::{Error, Result};

Expand All @@ -24,3 +27,32 @@ pub fn find_collection_object_type(
let collection = find_collection(configuration, collection_name)?;
Ok(collection.collection_type.clone())
}

pub fn unique_type_name<A, B>(
object_types: &BTreeMap<ObjectTypeName, A>,
added_object_types: &BTreeMap<ObjectTypeName, B>,
desired_type_name: &str,
) -> ObjectTypeName {
let (name, mut counter) = parse_counter_suffix(desired_type_name);
let mut type_name: ObjectTypeName = name.as_ref().into();
while object_types.contains_key(&type_name) || added_object_types.contains_key(&type_name) {
counter += 1;
type_name = format!("{desired_type_name}_{counter}").into();
}
type_name
}

/// [unique_type_name] adds a `_n` numeric suffix where necessary. There are cases where we go
/// through multiple layers of unique names. Instead of accumulating multiple suffixes, we can
/// increment the existing suffix. If there is no suffix then the count starts at zero.
pub fn parse_counter_suffix(name: &str) -> (Cow<'_, str>, u32) {
let re = Regex::new(r"^(.*?)_(\d+)$").unwrap();
let Some(captures) = re.captures(name) else {
return (Cow::Borrowed(name), 0);
};
let prefix = captures.get(1).unwrap().as_str();
let Some(count) = captures.get(2).and_then(|s| s.as_str().parse().ok()) else {
return (Cow::Borrowed(name), 0);
};
(Cow::Owned(prefix.to_string()), count)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So a suffix only gets added if there is more then one? Or does this not really matter because the prune fns below remove these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the suffix is only added if the count is 1 or higher. And then the prune step attempts to remove the suffix if that wouldn't cause a collision, but otherwise uses the lowest-number suffix that doesn't cause a collision after pruning.

3 changes: 2 additions & 1 deletion crates/cli/src/native_query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pub mod error;
mod helpers;
mod pipeline;
mod pipeline_type_context;
mod prune_object_types;
mod reference_shorthand;
mod type_constraint;
mod type_solver;
Expand Down Expand Up @@ -206,7 +207,7 @@ mod tests {
pipeline.clone(),
)?;

let expected_document_type_name: ObjectTypeName = "selected_title_documents_2".into();
let expected_document_type_name: ObjectTypeName = "selected_title_documents".into();

let expected_object_types = [(
expected_document_type_name.clone(),
Expand Down
2 changes: 1 addition & 1 deletion crates/cli/src/native_query/pipeline/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ mod tests {
let config = mflix_config();
let pipeline_types = infer_pipeline_types(&config, "documents", None, &pipeline).unwrap();
let expected = [(
"documents_documents_2".into(),
"documents_documents".into(),
ObjectType {
fields: [
(
Expand Down
52 changes: 33 additions & 19 deletions crates/cli/src/native_query/pipeline_type_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ use ndc_models::{ArgumentName, ObjectTypeName};

use super::{
error::{Error, Result},
helpers::unique_type_name,
prune_object_types::prune_object_types,
type_constraint::{ObjectTypeConstraint, TypeConstraint, TypeVariable, Variance},
type_solver::unify,
};
Expand Down Expand Up @@ -106,17 +108,12 @@ impl PipelineTypeContext<'_> {
e => e,
})?;

let result_document_type = variable_types
let mut result_document_type = variable_types
.get(&result_document_type_variable)
.expect("missing result type variable is missing");
let result_document_type_name = match result_document_type {
Type::Object(type_name) => type_name.clone().into(),
t => Err(Error::ExpectedObject {
actual_type: t.clone(),
})?,
};
.expect("missing result type variable is missing")
.clone();

let parameter_types = self
let mut parameter_types: BTreeMap<ArgumentName, Type> = self
.parameter_types
.into_iter()
.map(|(parameter_name, type_variable)| {
Expand All @@ -127,10 +124,31 @@ impl PipelineTypeContext<'_> {
})
.collect();

// Prune added object types to remove types that are not referenced by the return type or
// by parameter types, and therefore don't need to be included in the native query
// configuration.
let object_types = {
let mut reference_types = std::iter::once(&mut result_document_type)
.chain(parameter_types.values_mut())
.collect_vec();
prune_object_types(
&mut reference_types,
&self.configuration.object_types,
added_object_types,
)?
};

let result_document_type_name = match result_document_type {
Type::Object(type_name) => type_name.clone().into(),
t => Err(Error::ExpectedObject {
actual_type: t.clone(),
})?,
};

Ok(PipelineTypes {
result_document_type: result_document_type_name,
parameter_types,
object_types: added_object_types,
object_types,
warnings: self.warnings,
})
}
Expand Down Expand Up @@ -185,15 +203,11 @@ impl PipelineTypeContext<'_> {
}

pub fn unique_type_name(&self, desired_type_name: &str) -> ObjectTypeName {
let mut counter = 0;
let mut type_name: ObjectTypeName = desired_type_name.into();
while self.configuration.object_types.contains_key(&type_name)
|| self.object_types.contains_key(&type_name)
{
counter += 1;
type_name = format!("{desired_type_name}_{counter}").into();
}
type_name
unique_type_name(
&self.configuration.object_types,
&self.object_types,
desired_type_name,
)
}

pub fn set_stage_doc_type(&mut self, doc_type: TypeConstraint) {
Expand Down
Loading
Loading