Skip to content
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

coding of NOT logical expressions #85

Closed
tkardi opened this issue Sep 30, 2019 · 8 comments
Closed

coding of NOT logical expressions #85

tkardi opened this issue Sep 30, 2019 · 8 comments

Comments

@tkardi
Copy link

tkardi commented Sep 30, 2019

A similar issue to #77. I managed to drill down that the offending line is

EXPRESSION ("[TIME]" eq 'NOW' AND NOT "[TYPE]" ~ "(something|completely|different)")

upon which mappyfile.open raises:

[...]
/../.local/lib/python3.6/site-packages/lark/lexer.py in lex(self, stream, newline_types, ignore_types)
    165                 break
    166             else:
--> 167                 raise UnexpectedCharacters(stream, line_ctr.char_pos, line_ctr.line, line_ctr.column, state=self.state)
    168 
    169 

UnexpectedCharacters: No terminal defined for '"' at line 145 col 42

		EXPRESSION ("[TIME]" eq 'NOW' AND NOT "[TYPE]" ~ "(something|completely|differ
                                        ^

as removing the NOT keyword lets the file to be parsed with no further exceptions.

Looking at the docs on logical expressions every part the statement should be bracketed separately. So it seems this mapfile (and many others) have been written using a wrong or incomplete syntax. But then again these have survived 10-15 years of usage in a production environment..
So I guess where I'm trying to drive with this is - are these mapfiles broken and should they be fixed, or is there something that should/could be done with the lalr grammar file?

@geographika
Copy link
Owner

geographika commented Oct 6, 2019

@tkardi - thanks for raising this. As there is no official spec for a Mapfile then I guess if they work then they meet the spec..!
I'll be raising a RFC for version 8 of MapServer to get a spec in place. There are several examples of Mapfiles with deprecated or undocumented syntax that work. A spec to validate against would help find and correct these.
Expressions were the nastiest part of the Mapfile to write a grammar for. I think the MapServer expression parser has no grammar and just reads in tokens.
I'll leave this open and raise a discussion on the MapServer dev list on if brackets should be required for all parts of an expression.

@geographika
Copy link
Owner

geographika commented Oct 14, 2019

@tkardi - your Mapfiles are correct, and I'll need to update the expression parsing in mappyfile. See answer from Steve Lime at https://lists.osgeo.org/pipermail/mapserver-dev/2019-October/015886.html

The docs are wrong or at least confusing IMHO. The only requirement is that the entire logical expression has a whole needs to be encased in a ()'s so the parser can find it. Sub-expressions do not have to be encased in brackets unless it is needed to establish precedence in terms of how the expression is evaluated. MapServer more closely matches SQL and I wouldn't want to limit that. I do think ()'s can help with readability but they aren't required save the outer ()'s.

I've added some MapServer tests to check these at MapServer/MapServer#5897 and an update to the docs at MapServer/MapServer-documentation#288

@tkardi
Copy link
Author

tkardi commented Oct 17, 2019

Thanks so much for pushing this forward. I have a whole bunch of mapfiles (which unfortunately I cannot share but) I can surely use them for testing expression parsing :) after the changes you talked about.

@geographika
Copy link
Owner

geographika commented Jan 12, 2020

Test (currently failing) added to check for this - test_class_not_expression_no_brackets

@geographika
Copy link
Owner

geographika commented Feb 8, 2020

Just to note (from @erezsh):

the priority between NOT, AND, & OR seems to be different from usual. Usually NOT is of higher priority, so "NOT X AND Y" should evaluate as (NOT X) AND Y.
Meanwhile, it seems that in the mapfile grammar, it is evaluated as NOT (X AND Y).

@geographika
Copy link
Owner

geographika commented Feb 8, 2020

@tkardi - if you get a chance could you see if this issue is resolved with the latest master branch?

@tkardi
Copy link
Author

tkardi commented Feb 13, 2020

Seems to be very OK:

$ cat /tmp/test.lyr
LAYER
    [...]
    CLASS
        MAXSCALE 6000
        EXPRESSION ("[TIME]" eq 'NOW' AND NOT "[TYPE]" ~ "(something|completely|different)")
            STYLE
                SYMBOL 'something'
            END
    END
END
$ pip install https://github.com/geographika/mappyfile/archive/master.zip
[..]
$ python
Python 3.6.9 (default, Nov  7 2019, 10:44:02) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import mappyfile
>>> print(mappyfile.__version__)
0.8.4
>>> f = mappyfile.open('/tmp/test.lyr')
>>> f['classes'][0]['expression']
'( ( "[TIME]" eq \'NOW\' ) AND NOT ( "[TYPE]" ~ "(something|completely|different)" ) )'
>>> 

Feel free to close this issue if ok with you :)

@geographika
Copy link
Owner

geographika commented Feb 13, 2020

Thanks for checking @tkardi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants