Skip to content

BUG: syntax error in hdf query with ts #15544

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

BUG: syntax error in hdf query with ts #15544

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cc977f0
BUG: syntax error in hdf query with ts
chris-b1 Mar 2, 2017
213585f
CLN: remove timetuple type check
chris-b1 Mar 2, 2017
946a48e
ERR: more strict HDFStore string comparison
chris-b1 Mar 3, 2017
7c7100d
expand test cases
chris-b1 Mar 3, 2017
8288dca
lint
chris-b1 Mar 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v0.20.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,7 @@ Bug Fixes
- Bug in ``pd.merge_asof()`` where ``left_index``/``right_index`` together caused a failure when ``tolerance`` was specified (:issue:`15135`)



- Bug in ``pd.read_hdf()`` passing a ``Timestamp`` to the ``where`` parameter with a non date column (:issue:`15492`)


- Bug in ``Series`` constructor when both ``copy=True`` and ``dtype`` arguments are provided (:issue:`15125`)
Expand Down
9 changes: 2 additions & 7 deletions pandas/computation/pytables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,10 +188,6 @@ def stringify(value):
if v.tz is not None:
v = v.tz_convert('UTC')
return TermValue(v, v.value, kind)
elif (isinstance(v, datetime) or hasattr(v, 'timetuple') or
kind == u('date')):
v = time.mktime(v.timetuple())
return TermValue(v, pd.Timestamp(v), kind)
elif kind == u('timedelta64') or kind == u('timedelta'):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jreback - any idea what this was supposed to be doing? This doesn't impact any tested behavior and not sure it ever would have worked.

Copy link
Contributor

Choose a reason for hiding this comment

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

this was some really old compat IIRC. (or maybe just copied it from somewhere :>)

improving test coverage by deletions!

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that isn't hit at all in the tests

Copy link
Contributor

Choose a reason for hiding this comment

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

there is another reference to timetuple in that same file, you can prob take that out too.

v = _coerce_scalar_to_timedelta_type(v, unit='s').value
return TermValue(int(v), v, kind)
Expand Down Expand Up @@ -558,9 +554,8 @@ def parse_back_compat(self, w, op=None, value=None):

# stringify with quotes these values
def convert(v):
if (isinstance(v, (datetime, np.datetime64,
timedelta, np.timedelta64)) or
hasattr(v, 'timetuple')):
if isinstance(v, (datetime, np.datetime64,
timedelta, np.timedelta64)):
return "'{0}'".format(v)
return v

Expand Down
19 changes: 19 additions & 0 deletions pandas/tests/io/test_pytables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5080,6 +5080,25 @@ def test_query_long_float_literal(self):
expected = df.loc[[1], :]
tm.assert_frame_equal(expected, result)

def test_query_ts_string_column(self):
# GH 15492
df = pd.DataFrame({'date': ['2014-01-01', '2014-01-02'],
'real_date': date_range('2014-01-01', periods=2),
'values': [1, 2]},
columns=['date', 'real_date', 'values'])

ts = pd.Timestamp('2014-01-01') # noqa

with ensure_clean_store(self.path) as store:
store.append('test', df, format='table', data_columns=True)

result = store.select('test', where='date > ts')
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, though on 2nd thought. shouldn't this raise? e.g. we are passing a string column and a datelike. (and we know the dtypes here). A datetimelike cannot be compared against anything but a datetime column by-definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. Right now we are really liberal in terms of comparison to string columns - (maybe due to pytables?) - basically anything that has a repr - none of the following raise either.

pd.read_hdf('store.h5', 'df', where='str_col > 2.2')
pd.read_hdf('store.h5', 'df', where='str_col > True')

class O:
    pass
o = O()
pd.read_hdf('store.h5', 'df', where='str_col > o')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be fine with all of those raising.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I would be fine with being more strict. At the very least it would raise rather than return an empty result. Prob need a few test cases......

self.assertTrue(result.empty)

result = store.select('test', where='real_date > ts')
expected = df.loc[[1], :]
tm.assert_frame_equal(expected, result)


class TestHDFComplexValues(Base):
# GH10447
Expand Down