[#600] prevent case-sensitive testing with BETWEEN where infeasible#818
[#600] prevent case-sensitive testing with BETWEEN where infeasible#818d-w-moore wants to merge 1 commit intoirods:mainfrom
Conversation
| def setUpClass(cls): | ||
| with helpers.make_session() as sess: | ||
| if cls._csq is None: | ||
| cls._csq = cls._query_columns_case_sensitive_by_default(sess) |
There was a problem hiding this comment.
Why does case_sensitive_testing_feasible use a tuple to check cls._csq on line 64?
_query_columns_case_sensitive_by_default returns a boolean which removes the need for a tuple. Am I misunderstanding?
There was a problem hiding this comment.
oh. The in (True,) thing? I could have just said == True, if that's any better. Lots of folk are allergic to that, but in Python it's meaningful because of the False != None gotcha.
I expect people to object, it's part of software developer psychology to hate it when people write Python that contains things like is True or == True. I don't mind arguing with an actual human because you can convince them usually to see the logic.
But what I also expect is that codacy and ruff will heartily object and I'll be wasting a good half hour, in aggregate, dealing with the fallout. But when I write that type of code, I mean it exactly the way I wrote it... Lol
There was a problem hiding this comment.
Yep. What you typed felt very intentional. Just trying to understand the python-isms.
Is the in (True,) style a common pattern in python?
There was a problem hiding this comment.
no, actually. Just my own perverse variation : ) . It kind of screams, "please review. and change as appropriate."
There was a problem hiding this comment.
In the early review process, I like having humans look at things. Then, and only afterward, let codacy and ruff have their turn. Saves a lot of useless machinations.
|
It just occurred to me. Why does the PRC have tests which care about the database in use at all? Tests which care about that sort of thing should be part of the server. |
This doesn't care about which DB, just whether the server is evincing default case-insensitive behavior at the outset. If it is, there's no point in running the BETWEEN tests that will predictably error out. If you agree more with Alan's temporary solution, in which we just comment those tests out so that they never run, I'm fine with that too. In that case, we just close this PR and forget about it. I rather prefer a situation in which these tests run by default. Unless they shouldn't. Since Postsgres is the rule for the usual test scenario (i.e. our current set of PRC-test Github Actions) I prefer to keep the changes of this PR. But that is only my preference. |
Are you suggesting that this might be a bug in the server? It does seem odd that GenQuery would yield different results depending on database flavor. Kind of defeats the purpose a little bit. |
I think what I've done in this PR is simply allow the BETWEEN operator to be tested independently of the LT, GT type comparisons that many on the Internet have asserted are equivalent (but see my caveat at the end of this comment.) So, we end up doing what I think was intended : make sure the operator is actually doing as-intended in interactions between client and server. Of course, it will be moot if we find out that sometimes in GenQuery/SQL. |
Not a server bug. Kory's original point was that BETWEEN is passed right to the database. aka, it's not the server's job to mediate between client expectations and various flavors of DB. Only to interface. True, @korydraughn ? |
|
Looks like postgres and mysql will both return https://dev.mysql.com/doc/refman/8.4/en/comparison-operators.html#operator_between : https://www.postgresql.org/docs/current/functions-comparison.html#FUNCTIONS-COMPARISON-PRED-TABLE : postgres offers https://www.postgresql.org/docs/current/functions-comparison.html#FUNCTIONS-COMPARISON-PRED-TABLE : So, i'm not sure the point of discussion about case-insensitivity... do we want to offer that? feels confusing to me. |
@alanking I don't necessarily think it's a bug. I'm pointing out that addressing behavioral differences for the BETWEEN keyword across databases feels like something that should be part of the core server tests rather than the PRC.
@d-w-moore I need to hear more. Does this make the PRC special?
True. Now, I'm wondering if we should enforce our own rules around BETWEEN. That said, let's discuss offline. |
Tests using the
BETWEENoperator are now skipped if they are predicated on being case sensitive and if the installed object catalog ignores alphabetic case for query conditions dependent on lexical order (as tested byBETWEEN,>,>=,<, and<=.)