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.

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

Jump to bottom

Revert explain attributes change #16004

Merged
merged 4 commits into from Mar 4, 2024
Merged

Revert explain attributes change #16004

merged 4 commits into from Mar 4, 2024

Conversation

adarshsanjeev
Copy link
Contributor

@adarshsanjeev adarshsanjeev commented Feb 29, 2024 •
edited

Reverts a change made to explain attributes made by #15689. This will maintain backward compatibility.
The targetDataSource attribute is changed to return a string containing the datasources name, or "extern" if the query is an export.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@cryptoe cryptoe added this to the 29.0.1 milestone Feb 29, 2024
cryptoe
cryptoe reviewed Feb 29, 2024
View reviewed changes
sql/src/main/java/org/apache/druid/sql/calcite/planner/IngestHandler.java Outdated
@@ -405,7 +405,7 @@ public ExplainAttributes explainAttributes()
{
return new ExplainAttributes(
DruidSqlReplace.OPERATOR.getName(),
targetDatasource,
targetDatasource.getType(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I find it weird that the type is returning the name of the table in tableDestination. Wdyt ? Should we adjust the inteface IngestDestination ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This does seem like a mistake.

@gianm
Copy link
Contributor

gianm commented Feb 29, 2024 •
edited

We did release #15689 in 29.0.0. It accidentally included a breaking API change, which is problematic. This PR reverts the breaking change, but since #15689 has already been released, that means this PR is also a breaking API change. We need to treat it as such. There's a few options:

  1. Don't make this change, just accept that Add export capabilities to MSQ with SQL syntax #15689 had an unintentional breaking change, and keep rolling with the new format.
  2. Make this change, and document in the release notes that 29.0.0 had a different format for targetDataSource from both prior and future versions.
  3. Provide a migration path: add a query context property that controls the format of targetDataSource and allow people to override it.

IMO, given that this API is not likely widely used, and 29.0.0 was just a single release, (2) is the best option. But we do need to consider the other options when reviewing. IMO, the new format from 29.0.0 was also not designed ideally, since it is a JSON object that is similar to, but not the same as, the native datasource object. For example, the table name is under the key tableName rather than name (as in the native datasource object). This also points to (2), reverting back to pre-29.0.0 behavior, being a good option.

@github-actions github-actions bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Mar 1, 2024
@adarshsanjeev adarshsanjeev mentioned this pull request Mar 4, 2024
Merged
cryptoe
cryptoe approved these changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@cryptoe cryptoe left a comment

Choose a reason for hiding this comment

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

Changes LGTM.
I agree with @gianm proposal. Lets go ahead with point #2.

@cryptoe cryptoe merged commit 93eeb05 into apache:master Mar 4, 2024
83 checks passed
cryptoe pushed a commit that referenced this pull request Mar 6, 2024
@adarshsanjeev
Update docs to document breaking change in explain attributes (#16031)
86ee778 
Updates the docs to include a breaking change made regarding the attributes returned by an explain query. The return type of the field targetDataSource was changed to an object instead of a string containing the data source name.

This change is only present in Druid 29, and not in future versions, so it has been documented here. The change has been reverted in future versions here: #16004
cryptoe pushed a commit to cryptoe/druid that referenced this pull request Mar 6, 2024
@adarshsanjeev @cryptoe
Revert explain attributes change to old behaviour. (apache#16004)
ae5d719 
* Revert explain attributes change

* Fix tests

* Fix tests

* Rename function
@cryptoe cryptoe mentioned this pull request Mar 6, 2024
Merged
kfaraz pushed a commit that referenced this pull request Mar 9, 2024
@cryptoe
[Backport] Revert explain attributes change to old behaviour. (#16004) (
af69658 
#16061)

Backport of #16004 to 29.0.1.
@cryptoe cryptoe mentioned this pull request Mar 21, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gianm gianm gianm left review comments

@cryptoe cryptoe cryptoe approved these changes

Assignees
No one assigned
Labels
Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 Area - Querying Design Review Release Notes
Projects
None yet
Milestone
29.0.1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@adarshsanjeev @gianm @cryptoe