Created attachment 38303 [details] case_insensitive_dstar.patch This patch makes textual search in D* case-insensitive and adds respective tests. The behavior now matches Excel 2016.
Corporate firewall currently blocks me from uploading the test file (\test-data\spreadsheet\DGet.xls). I'll probably manage to do so tomorrow.
Comment on attachment 38303 [details] case_insensitive_dstar.patch Could you add some test coverage? Could you also provide some links to M$ docs that back up your assertion that this should be case insensitive?
Any interest is working on the TODO items in DStarRunner? * TODO: * - wildcards ? and * in string conditions * - functions as conditions
Created attachment 38304 [details] DGet.xls As promised yesterday I managed to upload the test file. To be placed in test-data\spreadsheet\DGet.xls
> Could you add some test coverage? As already promised yesterday I just uploaded the missing test file. > Could you also provide some links to M$ docs that back up your assertion that this should be case insensitive? This is the official documentation for DGET: https://support.microsoft.com/en-us/office/dget-function-455568bf-4eef-45f7-90f0-ec250d00892e (As expected,) the documentation is lacking and does not at all specify the exact semanticts of the function. Also I don't think it worth much to try to implement by documentation. Most important is to be compatible with Excels real behavior itself. And for that there is the test file I just uploaded. :-) I verified with Excel 2016, but I don't have access to any other excel versions currently. I would be surprised though if different excel versions would behave differently in this regard.
Comment on attachment 38303 [details] case_insensitive_dstar.patch You have bot provided a java unit test that uses the DGet xlsx - so there will be zero test coverage of this change
(In reply to PJ Fanning from comment #3) > Any interest is working on the TODO items in DStarRunner? > > * TODO: > * - wildcards ? and * in string conditions > * - functions as conditions I'd like to. But currently my POI work is sponsored by my company, thus I'm mostly implementing what we are using there. If the demand for more elaborate D* crops up, I'll definitely be on it. Just in case you didn't realize, I'm the original implementer of the D* funcions.
(In reply to PJ Fanning from comment #6) > Comment on attachment 38303 [details] > case_insensitive_dstar.patch > > You have bot provided a java unit test that uses the DGet xlsx - so there > will be zero test coverage of this change The test-data\spreadsheet\DGet.xls already exists in the repo and the corresponding src\test\java\org\apache\poi\ss\formula\functions\TestDGetFunctionsFromSpreadsheet.java is there already as well. I have just updated the test file.
In your patch, you use `.toUpperCase().toLowerCase()` - why both? Also, this will fail our code quality checks - we do not allow toUpperCase or toLowerCase without providing a Locale. Have you run a build of this change? The build target is called `forbiddenapis`.
I added r1901229 as a preliminary change - I only used toLowerCase in my patch.
(In reply to PJ Fanning from comment #9) > In your patch, you use `.toUpperCase().toLowerCase()` - why both? I looked up what `.equalsIgnoreCase()` is doing (https://javadoc.scijava.org/Java11/java.base/java/lang/String.html#equalsIgnoreCase(java.lang.String)) and tried to mimic that. I don't know the deeper reason behind it though. > Also, this will fail our code quality checks - we do not allow toUpperCase > or toLowerCase without providing a Locale. Have you run a build of this > change? The build target is called `forbiddenapis`. Actually I didn't do a full build after the change. I only ran the tests. I tried it now. As you said, the build does actually error. Thanks for pointing that out. I'll try to do a full build *before* next time.
(In reply to PJ Fanning from comment #10) > I added r1901229 as a preliminary change - I only used toLowerCase in my > patch. Thank you!
I added a DGET test using r1901237 - ignore the DCOUNT stuff in this revision (it's an experiment and the code might be removed). The DGET test is based on the M$ docs and it fails - so it appears that the existing DGET implementation is not correct.
I found the issue in the new DGET test so the test is ok now
I found that the current code does not support providing the field name (2nd param) as a number - Excel supports this. I added a fix using r1901250
I've added DCOUNT support - tracked as https://bz.apache.org/bugzilla/show_bug.cgi?id=66090