Skip to content

Improve project_requires method #210

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

Merged
merged 1 commit into from
Jun 27, 2025
Merged

Improve project_requires method #210

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
135 changes: 79 additions & 56 deletions lib/query-plan-executor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use query_planner::{
};
use serde::{Deserialize, Serialize};
use serde_json::{Map, Value};
use std::fmt::Write;
use std::{collections::HashMap, vec};
use tracing::{instrument, trace, warn}; // For reading file in main

Expand Down Expand Up @@ -92,7 +93,7 @@ fn process_errors_and_extensions(
}

#[instrument(
level = "debug",
level = "debug",
skip_all
name = "process_representations_result",
fields(
Expand Down Expand Up @@ -180,7 +181,7 @@ struct ProjectRepresentationsResult {
#[async_trait]
impl ExecutableFetchNode for FetchNode {
#[instrument(
level = "trace",
level = "trace",
skip_all,
name="FetchNode::execute_for_root",
fields(
Expand Down Expand Up @@ -252,7 +253,7 @@ impl ExecutableFetchNode for FetchNode {
}

#[instrument(
level = "debug",
level = "debug",
skip_all,
name = "execute_for_projected_representations",
fields(
Expand Down Expand Up @@ -852,12 +853,62 @@ pub struct QueryPlanExecutionContext<'a> {
}

impl QueryPlanExecutionContext<'_> {
fn project_requires_map(
pub fn project_requires(
&self,
requires_selections: &Vec<SelectionItem>,
entity: &Value,
) -> String {
// Pre-allocate a buffer, but we can do it without I think
let mut buffer = String::with_capacity(1024);
self.project_requires_mut(requires_selections, entity, &mut buffer);
buffer
Comment on lines +856 to +864
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jun 27, 2025

Choose a reason for hiding this comment

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

medium

Consider making the buffer a field of the QueryPlanExecutionContext struct to avoid reallocating it for each call. This could improve performance by reusing the same buffer across multiple calls to project_requires. Also, the comment "Pre-allocate a buffer, but we can do it without I think" suggests that the initial capacity might not be necessary or optimal. Consider benchmarking with and without the initial capacity to determine the best approach.

        &self,
        requires_selections: &Vec<SelectionItem>,
        entity: &Value,
    ) -> String {
        self.project_requires_mut(requires_selections, entity, &mut self.buffer);
        self.buffer.clone()
    }

}

fn project_requires_mut(
&self,
requires_selections: &Vec<SelectionItem>,
entity: &Value,
buffer: &mut String,
) {
if requires_selections.is_empty() {
// No selections, so serialize the entity directly into the buffer.
write!(buffer, "{}", serde_json::to_string(entity).unwrap()).unwrap();
return;
}

match entity {
Value::Null => buffer.push_str("null"),
Value::Bool(b) => write!(buffer, "{}", b).unwrap(),
Value::Number(n) => write!(buffer, "{}", n).unwrap(),
Value::String(s) => write!(buffer, "{}", serde_json::to_string(s).unwrap()).unwrap(),
Value::Array(entity_array) => {
buffer.push('[');
let mut first = true;
for entity_item in entity_array {
if !first {
buffer.push(',');
}
self.project_requires_mut(requires_selections, entity_item, buffer);
first = false;
}
buffer.push(']');
}
Value::Object(entity_obj) => {
buffer.push('{');
let mut first = true;
self.project_requires_map_mut(requires_selections, entity_obj, buffer, &mut first);
buffer.push('}');
}
}
}

fn project_requires_map_mut(
&self,
requires_selections: &Vec<SelectionItem>,
entity_obj: &Map<String, Value>,
) -> Vec<String> {
let mut items = vec![];
buffer: &mut String,
first: &mut bool,
) {
for requires_selection in requires_selections {
match &requires_selection {
SelectionItem::Field(requires_selection) => {
Expand All @@ -866,10 +917,21 @@ impl QueryPlanExecutionContext<'_> {
let original = entity_obj
.get(field_name)
.unwrap_or(entity_obj.get(response_key).unwrap_or(&Value::Null));
let projected_value =
self.project_requires(&requires_selection.selections.items, original);
if projected_value != "null" && !projected_value.is_empty() {
items.push("\"".to_string() + response_key + "\":" + &projected_value)

// To avoid writing empty fields, we write to a temporary buffer first
let mut temp_buffer = String::new();
self.project_requires_mut(
&requires_selection.selections.items,
original,
&mut temp_buffer,
);
Comment on lines +921 to +927
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jun 27, 2025

Choose a reason for hiding this comment

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

medium

Creating a new String in each iteration can be inefficient. Consider using String::clear() on temp_buffer at the beginning of the loop to reuse the same buffer, or even better, use the main buffer directly with proper management of the first flag.

                    let original = entity_obj
                        .get(field_name)
                        .unwrap_or(entity_obj.get(response_key).unwrap_or(&Value::Null));

                    // To avoid writing empty fields, we write to a temporary buffer first
                    temp_buffer.clear();
                    self.project_requires_mut(
                        &requires_selection.selections.items,
                        original,
                        &mut temp_buffer,
                    );


if temp_buffer != "null" && !temp_buffer.is_empty() {
if !*first {
buffer.push(',');
}
write!(buffer, "\"{}\":{}", response_key, temp_buffer).unwrap();
*first = false;
}
}
SelectionItem::InlineFragment(requires_selection) => {
Expand All @@ -882,55 +944,16 @@ impl QueryPlanExecutionContext<'_> {
type_name,
&requires_selection.type_condition,
) {
let projected = self
.project_requires_map(&requires_selection.selections.items, entity_obj);
// Merge the projected value into the result
if !projected.is_empty() {
items.extend(projected);
}
// If the projected value is not an object, it will be ignored
self.project_requires_map_mut(
&requires_selection.selections.items,
entity_obj,
buffer,
first,
);
}
}
}
}
items
}
pub fn project_requires(
&self,
requires_selections: &Vec<SelectionItem>,
entity: &Value,
) -> String {
if requires_selections.is_empty() {
return serde_json::to_string(entity).unwrap(); // No selections to project, return the entity as is
}
match entity {
Value::Null => "null".to_string(),
Value::Array(entity_array) => {
let mut items = Vec::with_capacity(entity_array.len());
for entity_item in entity_array {
let projected_value = self.project_requires(requires_selections, entity_item);
if projected_value != "null" && !projected_value.is_empty() {
items.push(projected_value);
}
}
"[".to_string() + &items.join(",") + "]"
}
Value::Object(entity_obj) => {
let items = self.project_requires_map(requires_selections, entity_obj);

if items.is_empty() {
return "null".to_string(); // No items to project, return null
}

// Join the items into a JSON object string
let projected_string = items.join(",");
"{".to_string() + &projected_string + "}"
}
Value::Bool(false) => "false".to_string(),
Value::Bool(true) => "true".to_string(),
Value::Number(num) => num.to_string(),
Value::String(string) => serde_json::to_string(string).unwrap(),
}
}
}

Expand Down Expand Up @@ -1006,7 +1029,7 @@ fn traverse_and_collect_mut<'a>(
}

#[instrument(
level = "trace",
level = "trace",
skip_all,
fields(
query_plan = ?query_plan,
Expand Down
Loading