Skip to content

Conversation

alex-spies
Copy link
Contributor

Debugging the analyzer and optimizers is easier when we focus only on the changes they make to the query plan. Let's add a separate sub-logger to RuleExecutor so we can selectively enable only logging of those changes.

Based on a nice suggestion by @nik9000 . Thanks, Nik!

Debugging the analyzer and optimizers is easier when we focus only on
the changes they make to the query plan. Let's add a separate sub-logger
to RuleExecutor so we can selectively enable only logging of those
changes.
@alex-spies alex-spies requested review from astefan and nik9000 August 6, 2025 15:39
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Aug 6, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

* }
* }</pre>
*/
private final Logger changeLog = LogManager.getLogger(getClass().getName() + ".changes");
Copy link
Member

Choose a reason for hiding this comment

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

You tested this locally, right? I'm pretty sure this is right, but I can't write code without some kind of mistake.

Copy link
Contributor Author

@alex-spies alex-spies Aug 7, 2025

Choose a reason for hiding this comment

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

++ confirmed locally that
a) this does not accidentally exclude the changes in query plans when turning trace logging on for logger.org.elasticsearch.xpack.esql, and
b) you can, indeed, enable the changes logger for each individual optimizer/analyzer, separately, using the instructions from the javadoc.

Ideally, we'd have a test for this, but I don't know that we already test the trace logs anywhere; setting up tests for this small change might be a bit high effort compared to the change.

@alex-spies alex-spies merged commit a13c7a4 into elastic:main Aug 7, 2025
33 checks passed
@alex-spies alex-spies deleted the add-rule-executor-logger-just-for-changes branch August 7, 2025 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants