Skip to content

Conversation

@MRichards99
Copy link
Contributor

Since my last PR, I went back and made some changes to my implementation of #86. I've created a new branch for this as we would like to target this for a 0.20.0 but also it was easier for me to start from a fresh branch.

The changes mean that when conditions are put into self.conditions, the key of the condition becomes a tuple:
{("title", "UPPER"): "like '%PolICE%'"}

This means that a user can create a condition in the same way that they do in 0.19.0. A user will not notice any difference with these changes, but they will be able to make use of the new feature if desired.

Where self.conditions is accessed, I have made changes to work with the new key format, particularly in __str__.

I've tried to address all of your comments of my previous implementation:

  • I've added a test in test_06_query.py (dac6a3d & ce68126) - this tests the functionality on an attribute as well as an attribute from a related entity. It uses the UPPER() function which is DataGateway's intended use case for this. I did also test LENGTH() for the tutorial which worked but didn't add this as a explicit test
  • I've created a dedicated function to split the attribute name from the function name, _split_db_functs (851fef2)
  • I've made sure the split function is only executed where for the query conditions (e141e54). It is only called in addConditions() as this is the only intended place for this feature for the moment. There are no calls in low level functions now.
  • I have also documented this in the docstring of addConditions() and the tutorial (c825b80)

@MRichards99 MRichards99 added the enhancement New feature or request label Oct 7, 2021
@MRichards99 MRichards99 requested a review from RKrahl October 7, 2021 17:00
@MRichards99 MRichards99 linked an issue Oct 7, 2021 that may be closed by this pull request
Copy link
Member

@RKrahl RKrahl left a comment

Choose a reason for hiding this comment

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

Many thanks for submitting this. I still have however a few issues:

  • most importantly, I'd like to keep the keys of query.conditions to be the attributes that the conditions are set on, see comment on line 414.
  • I'd prefer the parsing in _split_db_functs() to be more explicit, using a regular expression.
  • as suggested in a comment in #87, it might make sense to allow JPQL functions also in order. We could also add that as we are on it.
  • some minor things as commented in the code.

There is also a case where the implementation fails:

# search for User whose name is longer than 11 characters and comes later the "C" in alphabethic order.
>>> query = Query(client, "User", conditions={"LENGTH(fullName)": "> 11", "fullName": "> 'C'"})
>>> str(query)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/abuild/test/python-icat-0.19.1.dev6+gc825b80/build/lib/icat/query.py", line 515, in __str__
    for a, jpql_funct in sorted(self.conditions.keys()):
TypeError: '<' not supported between instances of 'NoneType' and 'str'

This seem to be related with using tuples of attribute names and JPQL functions in the keys of query.conditions, e.g. my first bullet above.

@RKrahl
Copy link
Member

RKrahl commented Oct 12, 2021

@MRichards99, note that as I already hinted before, you don't need to do all the work alone. I have an idea on how to organize query.conditions to include the JPQL functions and might come up with a suggestion for an implementation this week.

@RKrahl
Copy link
Member

RKrahl commented Oct 12, 2021

Supersedes and closes #87.

@RKrahl RKrahl mentioned this pull request Oct 12, 2021
@RKrahl RKrahl added this to the 0.20.0 milestone Oct 13, 2021
even if there is only one single condition for an attribute.
the keys, but rather set the values to a template string including the
JPQL function when present.
@MRichards99
Copy link
Contributor Author

Thanks for looking at this. I understand each of your points and thank you for starting to make some additional changes in #92. I will be happy to review your changes once you've finished them, just make the PR ready for review when you're finished :)

@RKrahl
Copy link
Member

RKrahl commented Oct 18, 2021

Basically, I assume that this fine now. I'll need to check it again though and there is also still some minor cleanup to do. This week is pretty busy with Virtual SciDataCon 2021, LEAPS plenary and ExPaNDS PID workshop, so I won't make any promisses when I'll get around to do this.

Copy link
Member

@RKrahl RKrahl left a comment

Choose a reason for hiding this comment

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

Looks good for me now.

@MRichards99
Copy link
Contributor Author

Brilliant, I'll let you merge this when you're ready if that's okay, just so we don't confuse each other regarding Python ICAT's release process.

@RKrahl
Copy link
Member

RKrahl commented Oct 25, 2021

Actually, I already merged it into release/0.20.0. This is the branch that collects all changes for 0.20.0 and will be merged into master at the moment of the release.

@RKrahl RKrahl merged commit ef02ebb into master Oct 29, 2021
@RKrahl RKrahl deleted the sql-functions-conditions branch October 29, 2021 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow SQL Functions to be used on Fields

3 participants