Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SI: Implement put operations #67
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
SI: Implement put operations #67
Changes from 27 commits
28c3a59
1b245b1
1239def
b605cce
3ed84d8
57b8a34
7812278
6b76439
3df7c89
8f0a02e
55525cb
157ac3d
0739ccc
d3a3651
72f917e
fba64b7
c27a3d6
19ca706
0167bd9
85e4d7c
713002d
fc06ef8
cafa17d
a508a1c
ce80df0
36885a4
c0c09d4
f612795
e609ef3
cdbe2d6
34a0362
c8a64c7
d48d3f3
e0037e0
3fa5d84
4824b68
469f35f
bdb948a
0261b7a
00d8a49
7a602e6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know
self.active_result_set
is introduced in this PR.so this is merely a generic question rather that specific to staging.
if we are using a field member
self.active_result_set
for keeping a state that means we won't be able to support multi threading in an application which concurrently uses pysql. is this understanding correct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by the first part of your question:
I don't believe this is correct.
active_result_set
has been present since the first version of this library. It's present onmain
right now.You're pulling on a valid thread. But I disagree with this assessment. In general pysql works fine with multi-threading. In fact, multi-threading is required if you want to cancel a running query (which is reflected in
PySQLCoreTestSuite.test_cancel_during_execute
).The specific scenario where
active_result_set
state would affect multi-threaded applications is if multiple threads are working with the same cursor. Is that a desirable usage pattern? I think there is usually one cursor per thread, in which case there's no issue with shared state.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if
uploads_base_path = /Users/user1
androw.localFile = /Users/user1/../user2
what does this method return?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please add some tests for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
tl;dr I updated the code in 34a0362 so that it resolves any relative paths before checking for their common_path. I added a test to prove this.
Before
/Users/user1
and/Users/user1/../user2
show a common path of/Users/user1
which is wrong.After
/Users/user1
and/Users/user1/../user2
show a common path of/Users
which is correct.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moderakh Are there other cases we should consider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question for reviewers: is there any specifically desired end-state for the
cursor
after a staging operation? Maybe we return a new NamedTupleStagingOperationResult
with properties of.successful:boolean
and perhaps a copy of theoperation
andlocalFile
that were used?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite get this question, but the cursor for now will return just one row and we should have reached the end of this cursor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@susodapop could you please with an a sample code explain how this will provide different experience to the end user?