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

Combine metadata fields #63

Merged

Conversation

apstndb
Copy link
Collaborator

@apstndb apstndb commented Jun 9, 2020

  • Combine scan_type and scan_target
  • Combine call_type, iterator_type, scan_type into node title
  • Hide subquery_cluster_node because spanner-cli doesn't display node id

Discussed in #57 (comment)

Examples

Example 1 Distributed Cross Apply(Back Join)

EXPLAIN SELECT * FROM Songs@{FORCE_INDEX=SongsBySongName}

Before

Query_Execution_Plan (EXPERIMENTAL)

.
+- Distributed Union (subquery_cluster_node: 1)
    +- Distributed Cross Apply (subquery_cluster_node: 15)
        +- [Input]  Create Batch 
        |   +- Distributed Union (subquery_cluster_node: 4, call_type: Local)
        |       +- Compute Struct 
        |           +- Scan (scan_type: IndexScan, scan_target: SongsBySongName, Full scan: true)
        +- [Map]  Serialize Result 
            +- Cross Apply 
                +- [Input]  Scan (scan_type: BatchScan, scan_target: $v2)
                +- [Map]  Distributed Union (call_type: Local, subquery_cluster_node: 23)
                    +- FilterScan 
                        +- Scan (scan_target: Songs, scan_type: TableScan)

After

Query_Execution_Plan (EXPERIMENTAL)

.
+- Distributed Union 
    +- Distributed Cross Apply 
        +- [Input]  Create Batch 
        |   +- Local Distributed Union 
        |       +- Compute Struct 
        |           +- Index Scan (Full scan: true, Index: SongsBySongName)
        +- [Map]  Serialize Result 
            +- Cross Apply 
                +- [Input]  Batch Scan (Batch: $v2)
                +- [Map]  Local Distributed Union 
                    +- FilterScan 
                        +- Table Scan (Table: Songs)

Example 2 Aggregation and Sort Limit

EXPLAIN SELECT SingerId, COUNT(*) AS SongCount
FROM Songs
GROUP BY SingerId
ORDER BY SongCount DESC
LIMIT 1

Before

Query_Execution_Plan (EXPERIMENTAL)

.
+- Serialize Result 
    +- Sort Limit (call_type: Global)
        +- Distributed Union (subquery_cluster_node: 3)
            +- Sort Limit (call_type: Local)
                +- Aggregate (iterator_type: Stream)
                    +- Distributed Union (subquery_cluster_node: 6, call_type: Local)
                        +- Scan (scan_type: IndexScan, Full scan: true, scan_target: SongsBySingerAlbumSongNameDesc)

After

Query_Execution_Plan (EXPERIMENTAL)

.
+- Serialize Result 
    +- Global Sort Limit 
        +- Distributed Union 
            +- Local Sort Limit 
                +- Stream Aggregate 
                    +- Local Distributed Union 
                        +- Index Scan (Index: SongsBySingerAlbumSongNameDesc, Full scan: true)

@googlebot googlebot added the cla: yes CLA signed label Jun 9, 2020
@apstndb apstndb requested a review from yfuruyama June 9, 2020 09:36
Copy link
Collaborator

@yfuruyama yfuruyama left a comment

Choose a reason for hiding this comment

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

LGTM for implementation. Can you add some tests?

@apstndb
Copy link
Collaborator Author

apstndb commented Jun 9, 2020

I have added basic test cases from the examples.

@apstndb apstndb requested a review from yfuruyama June 9, 2020 15:19
@apstndb
Copy link
Collaborator Author

apstndb commented Jun 9, 2020

It seems better to use google.golang.org/protobuf/types/known/structpb because github.com/golang/protobuf/ptypes/struct v1.4.0 provides only alias.

query_plan.go Outdated
fields = append(fields, fmt.Sprintf("%s: %s", k, v.GetStringValue()))
for k, v := range metadata {
switch k {
case "call_type", "iterator_type": // Skip because it is displayed in node title
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like implementations of getNodeTitle() and getAllMetadataString() are coupled with each other.
(getAllMetadataString() depends on the implementation of getNodeTitle()).

How about merging them into a single function?

func (n *Node) String() string {
  // assemble operator
  ...
  // assemble metadata
  ...
  return operator + metadata
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 8d7a1b4

@apstndb apstndb requested a review from yfuruyama June 9, 2020 16:08
Copy link
Collaborator

@yfuruyama yfuruyama left a comment

Choose a reason for hiding this comment

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

LGTM!

@yfuruyama yfuruyama merged commit b83c050 into cloudspannerecosystem:master Jun 9, 2020
@apstndb apstndb deleted the feature/combine-metadata branch June 9, 2020 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants