Update column + temporal filtering #19
Conversation
…her extra columns
|
After we merge this, I will add the CLI options and parsing of relative time. |
…her extra columns
sirupsen
left a comment
sirupsen
left a comment
There was a problem hiding this comment.
Couple of comments, but basically looks good to me—you can merge this without me looking at it again if you prefer
| yield Compare("<", self.key_column, str(self.end)) | ||
| def __post_init__(self): | ||
| if not self.update_column and (self.min_time or self.max_time): | ||
| raise ValueError("Error: min_time/max_time feature requires to specify 'update_column'") |
| if self.min_time is not None: | ||
| yield Compare("<=", Time(self.min_time), self.update_column) | ||
| if self.max_time is not None: | ||
| yield Compare("<", self.update_column, Time(self.max_time)) |
There was a problem hiding this comment.
A Python question: Why is yield preferred here to just returning a list? Because it's annoying to merge the lists I'm assuming, versus adding generators together and then materializing them to a list like you're doing below? Kinda cool pattern.
There was a problem hiding this comment.
Yes, it's just a cleaner syntax, saving me from creating a list, appending to it, and returning it.
| if self.start_key is not None: | ||
| yield Compare("<=", str(self.start_key), self.key_column) | ||
| if self.end_key is not None: | ||
| yield Compare("<", self.key_column, str(self.end_key)) |
There was a problem hiding this comment.
I think the key version will also benefit from a CLI arg 👍🏻 Thanks for changing its name and making it symmetric with time
Lots of people will probably instead of temporal just get the id of the last records when they did the last insertion, then do the check based on that
There was a problem hiding this comment.
We already have -k. Do you mean something different?
| } | ||
| """) | ||
| self.now = now = arrow.get(self.preql.now()) | ||
| self.preql.add(now.shift(days=-50), "50 days ago") |
There was a problem hiding this comment.
When I try to run your tests, this line errors (and presumably the others will too):
Would love if preql gave some more useful output here 😅
There was a problem hiding this comment.
This doesn't look related to preql in any way. What is Python.Framework? Never heard of it.
| assert b.count == 5 | ||
|
|
||
| assert not list(differ.diff_tables(a, a)) | ||
| self.assertEqual( len( list(differ.diff_tables(a, b)) ), 1 ) |
There was a problem hiding this comment.
Remember to run black(1), which'll probably complain about the spaces here
| assert not list(differ.diff_tables(a, a)) | ||
| self.assertEqual( len( list(differ.diff_tables(a, b)) ), 1 ) | ||
|
|
||
| def test_offset(self): |
There was a problem hiding this comment.
What about a test where the diff actually passes when you pass a range?
There was a problem hiding this comment.
That's what I do at the end of test_offset, unless you mean something else
Tiny fix for PR #19 - Optimizer hints v1
No description provided.