Skip to content

Commit be89cdb

Browse files
denikdenik
authored
dyn/convert: Fix ForceSendFields handling for embedded structs (#3650)
## Changes Fix ToTyped / FromTyped wrt ForceSendFields when there are (multiple) embedded structs. ## Why Incorrect conversion between typed & dynamic value can result in subtle bugs. In direct engine it is even more important, as we rely on typed structs more here. Enables #3646 ## Tests New unit tests.
1 parent c240095 commit be89cdb

File tree

10 files changed

+664
-33
lines changed

10 files changed

+664
-33
lines changed

‎acceptance/bundle/resources/jobs/update_single_node/databricks.yml‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
resources:
2+
jobs:
3+
foo:
4+
name: foo
5+
6+
trigger:
7+
periodic:
8+
interval: 1
9+
unit: DAYS
10+
11+
job_clusters:
12+
- job_cluster_key: key
13+
new_cluster:
14+
# main intention is to test this "num_workers: 0" as there were issues with zero conversion
15+
# ../update also tests this but there num_workers is implicitly set
16+
num_workers: 0
17+
spark_version: 13.3.x-scala2.12
18+
spark_conf:
19+
spark.databricks.cluster.profile: singleNode
20+
spark.master: local[*]
21+
custom_tags:
22+
ResourceClass: SingleNode

‎acceptance/bundle/resources/jobs/update_single_node/out.test.toml‎

Lines changed: 5 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎acceptance/bundle/resources/jobs/update_single_node/output.txt‎

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
2+
>>> [CLI] bundle plan
3+
create jobs.foo
4+
5+
Plan: 1 to add, 0 to change, 0 to delete, 0 unchanged
6+
7+
>>> [CLI] bundle debug plan
8+
{
9+
"plan": {
10+
"resources.jobs.foo": {
11+
"action": "create"
12+
}
13+
}
14+
}
15+
16+
>>> [CLI] bundle deploy
17+
Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files...
18+
Deploying resources...
19+
Updating deployment state...
20+
Deployment complete!
21+
22+
>>> [CLI] bundle debug plan
23+
{
24+
"plan": {
25+
"resources.jobs.foo": {
26+
"action": "skip"
27+
}
28+
}
29+
}
30+
31+
>>> print_requests
32+
{
33+
"body": {
34+
"deployment": {
35+
"kind": "BUNDLE",
36+
"metadata_file_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/metadata.json"
37+
},
38+
"edit_mode": "UI_LOCKED",
39+
"format": "MULTI_TASK",
40+
"job_clusters": [
41+
{
42+
"job_cluster_key": "key",
43+
"new_cluster": {
44+
"custom_tags": {
45+
"ResourceClass": "SingleNode"
46+
},
47+
"num_workers": 0,
48+
"spark_conf": {
49+
"spark.databricks.cluster.profile": "singleNode",
50+
"spark.master": "local[*]"
51+
},
52+
"spark_version": "13.3.x-scala2.12"
53+
}
54+
}
55+
],
56+
"max_concurrent_runs": 1,
57+
"name": "foo",
58+
"queue": {
59+
"enabled": true
60+
},
61+
"trigger": {
62+
"pause_status": "UNPAUSED",
63+
"periodic": {
64+
"interval": 1,
65+
"unit": "DAYS"
66+
}
67+
}
68+
},
69+
"method": "POST",
70+
"path": "/api/2.2/jobs/create"
71+
}
72+
jobs foo id='[JOB_ID]' name='foo'
73+
74+
=== Update trigger.periodic.unit and re-deploy
75+
>>> update_file.py databricks.yml DAYS HOURS
76+
77+
>>> [CLI] bundle plan
78+
update jobs.foo
79+
80+
Plan: 0 to add, 1 to change, 0 to delete, 0 unchanged
81+
82+
>>> [CLI] bundle debug plan
83+
{
84+
"plan": {
85+
"resources.jobs.foo": {
86+
"action": "update"
87+
}
88+
}
89+
}
90+
91+
>>> [CLI] bundle deploy
92+
Uploading bundle files to /Workspace/Users/[USERNAME]/.bundle/test-bundle/default/files...
93+
Deploying resources...
94+
Updating deployment state...
95+
Deployment complete!
96+
97+
>>> print_requests
98+
{
99+
"body": {
100+
"job_id": [JOB_ID],
101+
"new_settings": {
102+
"deployment": {
103+
"kind": "BUNDLE",
104+
"metadata_file_path": "/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/metadata.json"
105+
},
106+
"edit_mode": "UI_LOCKED",
107+
"format": "MULTI_TASK",
108+
"job_clusters": [
109+
{
110+
"job_cluster_key": "key",
111+
"new_cluster": {
112+
"custom_tags": {
113+
"ResourceClass": "SingleNode"
114+
},
115+
"num_workers": 0,
116+
"spark_conf": {
117+
"spark.databricks.cluster.profile": "singleNode",
118+
"spark.master": "local[*]"
119+
},
120+
"spark_version": "13.3.x-scala2.12"
121+
}
122+
}
123+
],
124+
"max_concurrent_runs": 1,
125+
"name": "foo",
126+
"queue": {
127+
"enabled": true
128+
},
129+
"trigger": {
130+
"pause_status": "UNPAUSED",
131+
"periodic": {
132+
"interval": 1,
133+
"unit": "HOURS"
134+
}
135+
}
136+
}
137+
},
138+
"method": "POST",
139+
"path": "/api/2.2/jobs/reset"
140+
}
141+
jobs foo id='[JOB_ID]' name='foo'
142+
143+
>>> [CLI] bundle plan
144+
Plan: 0 to add, 0 to change, 0 to delete, 1 unchanged
145+
146+
=== Fetch job ID and verify remote state
147+
>>> [CLI] jobs get [JOB_ID]
148+
{
149+
"job_id":[JOB_ID],
150+
"settings": {
151+
"deployment": {
152+
"kind":"BUNDLE",
153+
"metadata_file_path":"/Workspace/Users/[USERNAME]/.bundle/test-bundle/default/state/metadata.json"
154+
},
155+
"edit_mode":"UI_LOCKED",
156+
"format":"MULTI_TASK",
157+
"job_clusters": [
158+
{
159+
"job_cluster_key":"key",
160+
"new_cluster": {
161+
"custom_tags": {
162+
"ResourceClass":"SingleNode"
163+
},
164+
"num_workers":0,
165+
"spark_conf": {
166+
"spark.databricks.cluster.profile":"singleNode",
167+
"spark.master":"local[*]"
168+
},
169+
"spark_version":"13.3.x-scala2.12"
170+
}
171+
}
172+
],
173+
"max_concurrent_runs":1,
174+
"name":"foo",
175+
"queue": {
176+
"enabled":true
177+
},
178+
"trigger": {
179+
"pause_status":"UNPAUSED",
180+
"periodic": {
181+
"interval":1,
182+
"unit":"HOURS"
183+
}
184+
}
185+
}
186+
}
187+
188+
=== Destroy the job and verify that it's removed from the state and from remote
189+
>>> [CLI] bundle destroy --auto-approve
190+
The following resources will be deleted:
191+
delete job foo
192+
193+
All files and directories at the following location will be deleted: /Workspace/Users/[USERNAME]/.bundle/test-bundle/default
194+
195+
Deleting files...
196+
Destroy complete!
197+
198+
>>> print_requests
199+
{
200+
"body": {
201+
"job_id": [JOB_ID]
202+
},
203+
"method": "POST",
204+
"path": "/api/2.2/jobs/delete"
205+
}
206+
State not found for jobs.foo
207+
208+
>>> musterr [CLI] jobs get [JOB_ID]
209+
Error: Not Found
210+
211+
Exit code (musterr): 1

‎acceptance/bundle/resources/jobs/update_single_node/script‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
echo "*" > .gitignore
2+
trace $CLI bundle plan
3+
trace $CLI bundle debug plan
4+
trace $CLI bundle deploy
5+
trace $CLI bundle debug plan
6+
7+
print_requests() {
8+
jq --sort-keys 'select(.method != "GET" and (.path | contains("/jobs")))' < out.requests.txt
9+
rm out.requests.txt
10+
read_state.py jobs foo id name
11+
}
12+
13+
trace print_requests
14+
15+
title "Update trigger.periodic.unit and re-deploy"
16+
trace update_file.py databricks.yml DAYS HOURS
17+
trace $CLI bundle plan
18+
trace $CLI bundle debug plan
19+
trace $CLI bundle deploy
20+
trace print_requests
21+
22+
trace $CLI bundle plan
23+
24+
title "Fetch job ID and verify remote state"
25+
26+
ppid=`read_id.py jobs foo`
27+
echo "$ppid:JOB_ID" >> ACC_REPLS
28+
29+
trace $CLI jobs get $ppid
30+
rm out.requests.txt
31+
32+
title "Destroy the job and verify that it's removed from the state and from remote"
33+
trace $CLI bundle destroy --auto-approve
34+
trace print_requests
35+
36+
trace musterr $CLI jobs get $ppid
37+
rm out.requests.txt

‎libs/dyn/convert/end_to_end_test.go‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,3 +113,59 @@ func TestAdditional(t *testing.T) {
113113
})
114114
})
115115
}
116+
117+
func TestEndToEndForceSendFields(t *testing.T) {
118+
type Inner struct {
119+
InnerField string `json:"inner_field"`
120+
ForceSendFields []string `json:"-"`
121+
}
122+
type Outer struct {
123+
OuterField string `json:"outer_field"`
124+
Inner
125+
}
126+
127+
// Test with zero value in embedded struct
128+
src := Outer{
129+
OuterField: "outer_value",
130+
Inner: Inner{
131+
InnerField: "", // Zero value
132+
ForceSendFields: []string{"InnerField"}, // Should be preserved
133+
},
134+
}
135+
136+
assertFromTypedToTypedEqual(t, src)
137+
}
138+
139+
func TestEndToEndPointerForceSendFields(t *testing.T) {
140+
type NewCluster struct {
141+
NumWorkers int `json:"num_workers"`
142+
SparkVersion string `json:"spark_version"`
143+
ForceSendFields []string `json:"-"`
144+
}
145+
type JobCluster struct {
146+
JobClusterKey string `json:"job_cluster_key"`
147+
NewCluster *NewCluster `json:"new_cluster"`
148+
}
149+
type JobSettings struct {
150+
JobClusters []JobCluster `json:"job_clusters"`
151+
Name string `json:"name"`
152+
ForceSendFields []string `json:"-"`
153+
}
154+
155+
// Test with zero value in pointer embedded struct (like acceptance test)
156+
src := JobSettings{
157+
Name: "test-job",
158+
JobClusters: []JobCluster{
159+
{
160+
JobClusterKey: "key",
161+
NewCluster: &NewCluster{
162+
NumWorkers: 0, // Zero value
163+
SparkVersion: "13.3.x-scala2.12",
164+
ForceSendFields: []string{"NumWorkers"}, // Should be preserved
165+
},
166+
},
167+
},
168+
}
169+
170+
assertFromTypedToTypedEqual(t, src)
171+
}

0 commit comments

Comments
 (0)