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

Query api #289

Closed
wants to merge 16 commits into from
Closed

Query api #289

wants to merge 16 commits into from

Conversation

Koustavd18
Copy link
Contributor

Update: Use of enhanced query API

using enhanced query api which will fetch the table headers along with data

src/api/query.ts Outdated Show resolved Hide resolved
src/hooks/useQueryLogs.ts Outdated Show resolved Hide resolved
src/@types/parseable/api/stream.ts Outdated Show resolved Hide resolved
@Koustavd18 Koustavd18 requested a review from balaji-jr August 10, 2024 09:39
src/api/constants.ts Outdated Show resolved Hide resolved
@nitisht nitisht closed this Aug 10, 2024
@nitisht nitisht reopened this Aug 10, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 10, 2024
@Koustavd18 Koustavd18 requested a review from balaji-jr August 12, 2024 05:37
@@ -0,0 +1,6 @@
export function paramsParser(queryParams: Record<string, string>) {
for (let i in queryParams) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Try sending multiple items in the queryParams object and verify whether you get the expected result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

current implemention will take a singe param which will append to the url with "?" in front of the param. For multiple params we can chain them using '&'

@Koustavd18 Koustavd18 requested a review from balaji-jr August 12, 2024 08:37
@@ -0,0 +1,14 @@
export function paramsParser(queryParams: Record<string, string>) {
let parsedParams = '';
Copy link
Contributor

@balaji-jr balaji-jr Aug 12, 2024

Choose a reason for hiding this comment

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

  1. Use reduce then we do not have to append an extra '&' and rmv it
  2. This function should accept null | undefined and return an empty string for invalid params so that we don't have to write conditional operators on each endpoint where we need a query string. Like this: https://github.com/parseablehq/console/pull/289/files#diff-dc3796761041251e6580415307413937f036d7143b52eb0a254085147ac758bcR8
  3. The '?' itself should be emitted from this function.
  4. If we make all these changes, the fn name is inappropriate. It should mention what it receives and what it returns.

@@ -7,6 +8,11 @@ export type LogStreamSchemaData = {
metadata: Record<string, string>;
};

export type LogStreamQueryWithFields = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a query. Rename the type.

// to optimize performace, it has been decided to round off the time at the given level
// so making the end-time inclusive
const optimizeEndTime = (endTime: Date) => {
return dayjs(endTime).add(1, 'minute').toDate();
};

export const getQueryLogs = (logsQuery: QueryLogs) => {
export const getQueryLogs = (logsQuery: QueryLogs, queryParams?: QueryParams) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should always receive an object. Empty {} if no params.

@@ -33,11 +33,11 @@ export const getQueryLogs = (logsQuery: QueryLogs) => {
);
};

export const getQueryResult = (logsQuery: LogsQuery, query = '') => {
export const getQueryResult = (logsQuery: LogsQuery, query = '', queryParams?: QueryParams) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should always receive an object. Empty {} if no params.


const data = logsQueryRes.data;
const data = logsQueryRes.data.records;
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 a possibility where server could just send you {} or null.
Verify you always get a result that contains a records array even if there is no data.

export const makeHeadersFromQueryFields = (queryResponse: LogStreamQueryWithFields | null): string[] => {
if (queryResponse) {
const { fields } = queryResponse;
return fields;
Copy link
Contributor

Choose a reason for hiding this comment

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

If its straight forward, why'd you need a separate function ?

setData: (
store: LogsStore,
data: Log[],
queryResponse: LogStreamQueryWithFields | null,
Copy link
Contributor

@balaji-jr balaji-jr Aug 12, 2024

Choose a reason for hiding this comment

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

data: Log[],
queryResponse: LogStreamQueryWithFields | null

With logs provider context, both deal with almost the same values. Rmv one with less significant.

@@ -272,7 +277,7 @@ type LogsStoreReducers = {
getUniqueValues: (data: Log[], key: string) => string[];
makeExportData: (data: Log[], headers: string[], type: string) => Log[];
setRetention: (store: LogsStore, retention: { description: string; duration: string }) => ReducerOutput;
setTableHeaders: (store: LogsStore, schema: LogStreamSchemaData) => ReducerOutput;
setTableHeaders: (store: LogsStore, queryResponse: LogStreamQueryWithFields) => ReducerOutput;
Copy link
Contributor

Choose a reason for hiding this comment

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

for setting table headers, why'd you need entire LogStreamQueryWithFields

const { data: existingData, custQuerySearchState, tableOpts } = store;
const { filteredData } = existingData;
const newHeaders =
filteredData && custQuerySearchState.isQuerySearchActive
? makeHeadersfromData(filteredData)
: makeHeadersFromSchema(schema);
: makeHeadersFromQueryFields(queryResponse);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's straight forward. You do not need a function here.

@@ -563,7 +573,7 @@ const setData = (store: LogsStore, data: Log[], schema: LogStreamSchemaData | nu
: filterAndSortData(tableOpts, data);
const newPageSlice = filteredData && getPageSlice(currentPage, tableOpts.perPage, filteredData);
const newHeaders =
isQuerySearchActive && activeMode === 'sql' ? makeHeadersfromData(data) : makeHeadersFromSchema(schema);
isQuerySearchActive && activeMode === 'sql' ? makeHeadersfromData(data) : makeHeadersFromQueryFields(queryResponse);
Copy link
Contributor

Choose a reason for hiding this comment

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

why'd you want to make headers from data when
isQuerySearchActive && activeMode === 'sql'

@balaji-jr
Copy link
Contributor

@Koustavd18
Also, we need a change to fetch the schema whenever we fetch logs.

@balaji-jr
Copy link
Contributor

@Koustavd18
To determine the exact type definition for LogStreamQueryWithFields,
Try different scenarios (no-data, diff time range, non-existing streams, etc) and send multiple queries, and examine the results. This change is critical since a normal response without any params is an array but with fields=true it is an object.
This object may or may not contain fields and data keys.

Stating the above, it's better to have a common query function, that will always return an object {fields: string[], records: Log[] }. Just to prevent when we expect an array where its going to be an object response.

@balaji-jr balaji-jr closed this Aug 13, 2024
@balaji-jr
Copy link
Contributor

This will be addressed in a separate pr.

@Koustavd18 Koustavd18 deleted the queryApi branch August 19, 2024 05:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants